From 72c18d4deb9440c839ad2fb35b0537fd993915e0 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:15:50 -0500 Subject: [PATCH 01/19] honeykrisp: map placed memory at the allocation size A VK_EXT_map_memory_placed mapping covered the whole BO. The BO cache can return a BO that is larger than the allocation, for example 0x104000 bytes for a 0x100000 byte request. The mapping then covered the page after the allocation in application memory. The conformance test writes guard bytes there and reads them back, and it failed with a crash. Record the length of the live mapping in the BO and use it to unmap. Map a placed mapping with the allocation size, not the BO size. Report VK_ERROR_MEMORY_MAP_FAILED when the map fails, in place of an assert that is compiled out in release builds. A placed mapping can also stay on a BO that goes back to the cache. The next placed map of that BO then returned the old address and made no new mapping. Unmap a placed mapping when its memory is freed, and drop any old mapping before a new placed map. The virtio path takes the new size argument. It was not run. --- src/asahi/lib/agx_bo.h | 5 +++++ src/asahi/lib/agx_device.c | 17 ++++++++++------- src/asahi/lib/agx_device.h | 27 +++++++++++++++++++++++---- src/asahi/lib/agx_device_virtio.c | 12 ++++++++---- src/asahi/vulkan/hk_device_memory.c | 29 ++++++++++++++++++++++------- src/asahi/vulkan/hk_device_memory.h | 6 ++++++ 6 files changed, 74 insertions(+), 22 deletions(-) diff --git a/src/asahi/lib/agx_bo.h b/src/asahi/lib/agx_bo.h index d9eed7ac5e1a..a2f35d205ce8 100644 --- a/src/asahi/lib/agx_bo.h +++ b/src/asahi/lib/agx_bo.h @@ -85,6 +85,11 @@ struct agx_bo { /* Suffixed to force agx_bo_map access */ void *_map; + /* Length of the live mapping in _map, or 0 when it is the whole BO. A + * placed mapping can be shorter than the BO. + */ + size_t _map_size; + /* Process-local index */ uint32_t handle; diff --git a/src/asahi/lib/agx_device.c b/src/asahi/lib/agx_device.c index f158e0e7acea..f64599ba1146 100644 --- a/src/asahi/lib/agx_device.c +++ b/src/asahi/lib/agx_device.c @@ -86,8 +86,7 @@ agx_bo_free(struct agx_device *dev, struct agx_bo *bo) { const uint64_t handle = bo->handle; - if (bo->_map) - munmap(bo->_map, bo->size); + agx_bo_unmap(bo); /* Free the VA. No need to unmap the BO or unbind the VA, as the kernel will * take care of that when we close it. @@ -228,7 +227,8 @@ agx_bo_alloc(struct agx_device *dev, size_t size, size_t align, } static void -agx_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr) +agx_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, + size_t size) { assert(bo->_map == NULL && "not double mapped"); @@ -243,14 +243,17 @@ agx_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr) } flags = MAP_SHARED | (fixed_addr ? MAP_FIXED : 0); - bo->_map = os_mmap(fixed_addr, bo->size, PROT_READ | PROT_WRITE, flags, - dev->fd, gem_mmap_offset.offset); + bo->_map = os_mmap(fixed_addr, size, PROT_READ | PROT_WRITE, flags, dev->fd, + gem_mmap_offset.offset); if (bo->_map == MAP_FAILED) { - bo->_map = NULL; fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i offset=0x%llx %m\n", - bo->_map, (long long)bo->size, dev->fd, + bo->_map, (long long)size, dev->fd, (long long)gem_mmap_offset.offset); + bo->_map = NULL; + bo->_map_size = 0; + } else { + bo->_map_size = size; } } diff --git a/src/asahi/lib/agx_device.h b/src/asahi/lib/agx_device.h index e80dd9b5a5e1..c75859f1b302 100644 --- a/src/asahi/lib/agx_device.h +++ b/src/asahi/lib/agx_device.h @@ -5,6 +5,7 @@ #pragma once +#include #include #include #include "drm-uapi/asahi_drm.h" @@ -76,7 +77,8 @@ typedef struct { enum agx_bo_flags flags); int (*bo_bind)(struct agx_device *dev, struct drm_asahi_gem_bind_op *ops, uint32_t count); - void (*bo_mmap)(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr); + void (*bo_mmap)(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, + size_t size); ssize_t (*get_params)(struct agx_device *dev, void *buf, size_t size); int (*submit)(struct agx_device *dev, struct drm_asahi_submit *submit, struct agx_submit_virt *virt); @@ -202,11 +204,28 @@ agx_rw_addr_to_ro(struct agx_device *dev, uint64_t addr) return addr + dev->sparse_ro_offset; } +static inline void +agx_bo_unmap(struct agx_bo *bo) +{ + if (bo->_map) { + munmap(bo->_map, bo->_map_size ? bo->_map_size : bo->size); + bo->_map = NULL; + bo->_map_size = 0; + } +} + +/* Map the first `size` bytes of the BO, at fixed_addr when it is not NULL. A + * mapping left by an earlier owner of a cached BO is dropped first, as it + * cannot be at the requested address. + */ static inline void * -agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr) +agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) { + if (fixed_addr) + agx_bo_unmap(bo); + if (!bo->_map) - bo->dev->ops.bo_mmap(bo->dev, bo, fixed_addr); + bo->dev->ops.bo_mmap(bo->dev, bo, fixed_addr, size); return bo->_map; } @@ -214,7 +233,7 @@ agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr) static inline void * agx_bo_map(struct agx_bo *bo) { - return agx_bo_map_placed(bo, NULL); + return agx_bo_map_placed(bo, NULL, bo->size); } static inline bool diff --git a/src/asahi/lib/agx_device_virtio.c b/src/asahi/lib/agx_device_virtio.c index 3b8f3aae87e2..488f4789654e 100644 --- a/src/asahi/lib/agx_device_virtio.c +++ b/src/asahi/lib/agx_device_virtio.c @@ -191,13 +191,17 @@ agx_virtio_bo_unbind_object(struct agx_device *dev, uint32_t object_handle) } static void -agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr) +agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, + size_t size) { - bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, bo->size, fixed_addr); + bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, size, fixed_addr); if (bo->_map == MAP_FAILED) { - bo->_map = NULL; fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i\n", bo->_map, - (long long)bo->size, dev->fd); + (long long)size, dev->fd); + bo->_map = NULL; + bo->_map_size = 0; + } else { + bo->_map_size = size; } } diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index dde1437a3aae..f17b440e9cf4 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -277,6 +277,12 @@ hk_FreeMemory(VkDevice device, VkDeviceMemory _mem, if (mem->bo->flags & (AGX_BO_SHAREABLE | AGX_BO_SHARED)) hk_remove_ext_bo(dev, mem->bo); + /* Do not hand a placed mapping, which lies in application address space, + * to the BO cache with the BO. + */ + if (mem->map && mem->placed_map) + agx_bo_unmap(mem->bo); + agx_bo_unreference(&dev->dev, mem->bo); vk_device_memory_destroy(&dev->vk, pAllocator, &mem->vk); @@ -330,8 +336,18 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, "Memory object already mapped."); } - mem->map = agx_bo_map_placed(mem->bo, fixed_addr); - assert(!fixed_addr || mem->map == fixed_addr); + /* A placed mapping covers exactly the allocation: the BO can be larger + * (BO cache, GPU page rounding) and the memory after the allocation belongs + * to the application. An ordinary mapping keeps the whole BO. + */ + mem->map = agx_bo_map_placed(mem->bo, fixed_addr, + fixed_addr ? mem->vk.size : mem->bo->size); + if (mem->map == NULL) { + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "Failed to map memory object."); + } + + mem->placed_map = fixed_addr != NULL; *ppData = mem->map + offset; return VK_SUCCESS; @@ -349,21 +365,20 @@ hk_UnmapMemory2KHR(VkDevice device, if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { VK_FROM_HANDLE(hk_device, dev, device); - void *err = mmap(mem->bo->_map, mem->bo->size, PROT_NONE, + void *err = mmap(mem->bo->_map, mem->bo->_map_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); if (err == MAP_FAILED) { return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Failed to map over original mapping"); } mem->bo->_map = NULL; + mem->bo->_map_size = 0; } else { - if (mem->bo->_map) { - munmap(mem->bo->_map, mem->bo->size); - mem->bo->_map = NULL; - } + agx_bo_unmap(mem->bo); } mem->map = NULL; + mem->placed_map = false; return VK_SUCCESS; } diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index 29d3651972ad..6a29324442d9 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -22,6 +22,12 @@ struct hk_device_memory { struct agx_bo *bo; void *map; + + /* The live mapping was made with VK_MEMORY_MAP_PLACED_BIT_EXT. Such a + * mapping belongs to this object, so it is dropped on unmap and on free + * instead of staying on a BO that returns to the BO cache. + */ + bool placed_map; }; VK_DEFINE_NONDISP_HANDLE_CASTS(hk_device_memory, vk.base, VkDeviceMemory, From f91b6f40b3c6c529fc523c3339060e0af165762f Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:28:18 -0500 Subject: [PATCH 02/19] honeykrisp: check placed map address, size, and sharing Review follow-up for the placed map change. Return VK_ERROR_MEMORY_MAP_FAILED if a placed map ends at another address than the one the application asked for, and drop that mapping. The old assert for this was compiled out in release builds. A placed map without a range covers the whole allocation from offset 0. Return an error for any other offset or size. Refuse a placed map when the BO is shared through an import and already mapped, so a mapping that another memory object holds is not torn down. Use one helper for the length of a live mapping in the unmap and the reserve paths. A reserve unmap of a BO with no mapping does nothing. --- src/asahi/lib/agx_bo.h | 5 ++-- src/asahi/lib/agx_device.h | 8 +++++- src/asahi/vulkan/hk_device_memory.c | 41 ++++++++++++++++++++++++----- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/asahi/lib/agx_bo.h b/src/asahi/lib/agx_bo.h index a2f35d205ce8..927929e7ec78 100644 --- a/src/asahi/lib/agx_bo.h +++ b/src/asahi/lib/agx_bo.h @@ -85,8 +85,9 @@ struct agx_bo { /* Suffixed to force agx_bo_map access */ void *_map; - /* Length of the live mapping in _map, or 0 when it is the whole BO. A - * placed mapping can be shorter than the BO. + /* Length of the live mapping in _map. Zero when the mapping was not made + * by bo_mmap (the trace wrapper), which covers the whole BO. A placed + * mapping can be shorter than the BO. */ size_t _map_size; diff --git a/src/asahi/lib/agx_device.h b/src/asahi/lib/agx_device.h index c75859f1b302..10a331a10d05 100644 --- a/src/asahi/lib/agx_device.h +++ b/src/asahi/lib/agx_device.h @@ -204,11 +204,17 @@ agx_rw_addr_to_ro(struct agx_device *dev, uint64_t addr) return addr + dev->sparse_ro_offset; } +static inline size_t +agx_bo_map_len(const struct agx_bo *bo) +{ + return bo->_map_size ? bo->_map_size : bo->size; +} + static inline void agx_bo_unmap(struct agx_bo *bo) { if (bo->_map) { - munmap(bo->_map, bo->_map_size ? bo->_map_size : bo->size); + munmap(bo->_map, agx_bo_map_len(bo)); bo->_map = NULL; bo->_map_size = 0; } diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index f17b440e9cf4..95f9ebefe02b 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -336,6 +336,24 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, "Memory object already mapped."); } + if (fixed_addr) { + /* memoryMapRangePlaced is false, so a placed map covers the whole + * allocation from offset 0. + */ + if (offset != 0 || size != mem->vk.size) { + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "A placed map must cover the whole allocation."); + } + + /* An imported BO can be shared by two memory objects. Do not tear down + * a mapping the other one may hold. + */ + if (mem->bo->_map && p_atomic_read(&mem->bo->refcnt) > 1) { + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "Memory is shared and already mapped."); + } + } + /* A placed mapping covers exactly the allocation: the BO can be larger * (BO cache, GPU page rounding) and the memory after the allocation belongs * to the application. An ordinary mapping keeps the whole BO. @@ -347,6 +365,13 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, "Failed to map memory object."); } + if (fixed_addr && mem->map != fixed_addr) { + agx_bo_unmap(mem->bo); + mem->map = NULL; + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "Mapped address does not match the placed address."); + } + mem->placed_map = fixed_addr != NULL; *ppData = mem->map + offset; @@ -365,14 +390,16 @@ hk_UnmapMemory2KHR(VkDevice device, if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { VK_FROM_HANDLE(hk_device, dev, device); - void *err = mmap(mem->bo->_map, mem->bo->_map_size, PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) { - return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to map over original mapping"); + if (mem->bo->_map) { + void *err = mmap(mem->bo->_map, agx_bo_map_len(mem->bo), PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (err == MAP_FAILED) { + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "Failed to map over original mapping"); + } + mem->bo->_map = NULL; + mem->bo->_map_size = 0; } - mem->bo->_map = NULL; - mem->bo->_map_size = 0; } else { agx_bo_unmap(mem->bo); } From ddfb4cf114a6e72b690581ff6fb2fe8447d11ffa Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:30:11 -0500 Subject: [PATCH 03/19] honeykrisp: bound a placed map by the BO size The placed length now comes from the allocation size. Return an error if it ever exceeds the BO, so mmap cannot map past the GEM object. --- src/asahi/vulkan/hk_device_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index 95f9ebefe02b..679c575217dd 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -340,7 +340,8 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, /* memoryMapRangePlaced is false, so a placed map covers the whole * allocation from offset 0. */ - if (offset != 0 || size != mem->vk.size) { + if (offset != 0 || size != mem->vk.size || + mem->vk.size > mem->bo->size) { return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "A placed map must cover the whole allocation."); } From ae0e7c95761d44fc4f4d978bcead10a8af4c3659 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:05:16 -0500 Subject: [PATCH 04/19] honeykrisp: add CPU regression for placed map vs retained image mapping A bound image retains an internal host-copy mapping (hk_image_plane_bind, plane->map = agx_bo_map(mem->bo) + offset) with no mem->map and no BO reference, so a later application placed map reaches agx_bo_map_placed, drops the BO mapping, and leaves the retained plane pointers dangling for the host-copy paths. The new libasahi_placed_map_test drives the real agx_device.h mapping code with the exact bind, placed-map, unmap and reserve expressions from hk_image.c and hk_device_memory.c. The only mock is the kernel mmap, which hands back anonymous pages. Cases: placed map then plain unmap, placed map then VK_MEMORY_UNMAP_RESERVE_BIT_EXT, and two images aliased on one memory object. Fails at ddfb4cf114a6; passes once the internal mapping is independent of the application mapping. --- src/asahi/lib/meson.build | 13 ++ src/asahi/lib/tests/test-placed-map.c | 305 ++++++++++++++++++++++++++ 2 files changed, 318 insertions(+) create mode 100644 src/asahi/lib/tests/test-placed-map.c diff --git a/src/asahi/lib/meson.build b/src/asahi/lib/meson.build index e50a70eff518..007ad4ec6285 100644 --- a/src/asahi/lib/meson.build +++ b/src/asahi/lib/meson.build @@ -69,6 +69,19 @@ libasahi_lib = static_library( ) if with_tests + test( + 'libasahi_placed_map_test', + executable( + 'libasahi_placed_map_test', + 'tests/test-placed-map.c', + c_args : [c_msvc_compat_args, no_override_init_args, + '-I' + join_paths(meson.current_source_dir(), '..', 'libagx')], + gnu_symbol_visibility : 'hidden', + dependencies : [idep_mesautil, idep_agx_pack], + include_directories : [inc_src, inc_asahi, inc_virtio_vdrm], + ), + suite : ['asahi'], + ) test( 'libasahi_tests', executable( diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c new file mode 100644 index 000000000000..db878a1798d2 --- /dev/null +++ b/src/asahi/lib/tests/test-placed-map.c @@ -0,0 +1,305 @@ +/* + * Copyright 2026 Joshua Warren + * SPDX-License-Identifier: MIT + */ + +/* + * CPU-only regression for the omacom/mesa#8 review finding + * (https://github.com/omacom/mesa/pull/8#discussion_r4418383484): + * + * A non-sparse image bind retains an internal host-copy mapping + * (hk_image_plane_bind, hk_image.c:1334: plane->map = agx_bo_map(mem->bo) + + * offset) without setting mem->map or taking a BO reference. A subsequent + * application vkMapMemory2KHR with VK_MEMORY_MAP_PLACED_BIT_EXT reaches + * agx_bo_map_placed (agx_device.h) with a fixed address, which unconditionally + * drops the BO's current mapping. The retained plane->map then dangles, and + * the host-copy paths (hk_image.c:1444, :1523, :1610) read and write through + * it. The plain unmap path (hk_device_memory.c:405) and the + * VK_MEMORY_UNMAP_RESERVE_BIT_EXT path (:394) invalidate the same retained + * pointer. + * + * This test drives the real production mapping code: agx_bo_map, + * agx_bo_map_placed and agx_bo_unmap exactly as defined in agx_device.h, with + * the exact call expressions used by hk_image_plane_bind, hk_MapMemory2KHR and + * hk_UnmapMemory2KHR. The only mock is the kernel mmap: mock_bo_mmap hands + * back anonymous pages for the request, like the DRM GEM mmap path does. There + * is no GPU, no device node and no Vulkan entrypoint; those are covered by the + * admitted device gate (dEQP-VK host-copy coverage with a placed map after + * image bind), not here. + * + * Fails at head ddfb4cf114a6: the retained image mapping is destroyed and the + * host-copy read faults or reads foreign memory. Passes once the internal + * image mapping is independent of the application mapping. + */ + +#include +#include +#include +#include +#include +#include + +#include "agx_device.h" + +/* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); + * a placed map covers the allocation only. Same numbers as the PR: 0x100000 + * allocation in a 0x104000 BO. */ +#define ALLOC_SIZE 0x100000 +#define BO_SIZE 0x104000 + +/* Two images (or planes) bound to one memory object at different offsets, as + * hk_BindImageMemory2 allows. */ +#define BIND_OFF_A 0x1000 +#define BIND_OFF_B 0x80000 + +#define CANARY_LEN 0x1000 + +struct mock_state { + struct agx_device dev; + struct agx_bo bo; +}; + +/* The single mock: the kernel side of a GEM mmap. Body mirrors agx_bo_mmap + * (agx_device.c) minus the ioctls. */ +static void +mock_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, + size_t size) +{ + (void)dev; + if (bo->_map != NULL) { + fprintf(stderr, "mock: bo double mapped\n"); + exit(99); + } + + bo->_map = mmap(fixed_addr, size, PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS | (fixed_addr ? MAP_FIXED : 0), + -1, 0); + if (bo->_map == MAP_FAILED) { + bo->_map = NULL; + bo->_map_size = 0; + } else { + bo->_map_size = size; + } +} + +static void +mock_bo_init(struct mock_state *m, size_t bo_size) +{ + memset(m, 0, sizeof(*m)); + m->dev.ops.bo_mmap = mock_bo_mmap; + m->bo.dev = &m->dev; + m->bo.size = bo_size; + m->bo.refcnt = 1; +} + +/* Grab a page-aligned address that is (almost certainly) free, so the placed + * map target cannot alias the retained mapping. */ +static void * +reserve_free_address(size_t size) +{ + void *p = mmap(NULL, size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (p == MAP_FAILED) + return NULL; + munmap(p, size); + return p; +} + +static sigjmp_buf fault_env; +static volatile sig_atomic_t faulted; + +static void +fault_handler(int sig) +{ + (void)sig; + faulted = 1; + siglongjmp(fault_env, 1); +} + +/* Reads through a possibly-dangling retained pointer the way the host-copy + * paths do (hk_image.c:1523). Returns false when the access faults. */ +static bool +guarded_copy(void *dst, const void *src, size_t n) +{ + void *prev_segv = signal(SIGSEGV, fault_handler); + void *prev_bus = signal(SIGBUS, fault_handler); + + faulted = 0; + if (sigsetjmp(fault_env, 1) == 0) + memcpy(dst, src, n); + + signal(SIGSEGV, prev_segv); + signal(SIGBUS, prev_bus); + return !faulted; +} + +/* Bind (hk_image.c:1333-1335), placed map (hk_device_memory.c:362), plain + * unmap (:405), host-copy read through the retained pointer (hk_image.c:1523). + */ +static int +placed_map_after_bind_then_unmap(void) +{ + struct mock_state m; + mock_bo_init(&m, BO_SIZE); + + uint8_t *image_map = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; + uint8_t pattern[CANARY_LEN]; + memset(pattern, 0x5A, sizeof(pattern)); + memcpy(image_map, pattern, sizeof(pattern)); + + void *placed = reserve_free_address(ALLOC_SIZE); + if (!placed) { + printf("SKIP placed_map_after_bind_then_unmap: no free address\n"); + return 0; + } + + uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + if (mem_map != placed) { + printf("FAIL placed_map_after_bind_then_unmap: placed map returned %p, " + "requested %p\n", + (void *)mem_map, placed); + return 1; + } + + agx_bo_unmap(&m.bo); + + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, image_map, CANARY_LEN)) { + printf("FAIL placed_map_after_bind_then_unmap: host copy through the " + "retained image mapping faults after placed map + unmap " + "(omacom/mesa#8 r4418383484)\n"); + return 1; + } + if (memcmp(readback, pattern, CANARY_LEN) != 0) { + printf("FAIL placed_map_after_bind_then_unmap: retained image mapping " + "was destroyed or reused: canary mismatch\n"); + return 1; + } + + printf("PASS placed_map_after_bind_then_unmap\n"); + return 0; +} + +/* Same sequence, but the application unmaps with + * VK_MEMORY_UNMAP_RESERVE_BIT_EXT: the production path (hk_device_memory.c:394) + * overlays the BO mapping with PROT_NONE, which must not touch the internal + * image mapping either. */ +static int +reserve_unmap_after_bind(void) +{ + struct mock_state m; + mock_bo_init(&m, BO_SIZE); + + uint8_t *image_map = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; + uint8_t pattern[CANARY_LEN]; + memset(pattern, 0xA5, sizeof(pattern)); + memcpy(image_map, pattern, sizeof(pattern)); + + void *placed = reserve_free_address(ALLOC_SIZE); + if (!placed) { + printf("SKIP reserve_unmap_after_bind: no free address\n"); + return 0; + } + + uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + if (mem_map != placed) { + printf("FAIL reserve_unmap_after_bind: placed map returned %p, " + "requested %p\n", + (void *)mem_map, placed); + return 1; + } + + /* Production reserve expression (hk_device_memory.c:395). */ + void *err = mmap(m.bo._map, agx_bo_map_len(&m.bo), PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (err == MAP_FAILED) { + printf("SKIP reserve_unmap_after_bind: reserve mmap failed\n"); + return 0; + } + m.bo._map = NULL; + m.bo._map_size = 0; + + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, image_map, CANARY_LEN)) { + printf("FAIL reserve_unmap_after_bind: host copy through the retained " + "image mapping faults after placed map + reserve unmap\n"); + return 1; + } + if (memcmp(readback, pattern, CANARY_LEN) != 0) { + printf("FAIL reserve_unmap_after_bind: retained image mapping was " + "destroyed: canary mismatch\n"); + return 1; + } + + printf("PASS reserve_unmap_after_bind\n"); + return 0; +} + +/* Two images bound to one memory object: one placed map must not invalidate + * either retained mapping (hk_BindImageMemory2 binds each plane of an aliasing + * image from the same mem->bo). */ +static int +aliased_images_survive_placed_map(void) +{ + struct mock_state m; + mock_bo_init(&m, BO_SIZE); + + uint8_t *image_a = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; + uint8_t *image_b = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_B; + uint8_t pattern_a[CANARY_LEN], pattern_b[CANARY_LEN]; + memset(pattern_a, 0x5A, sizeof(pattern_a)); + memset(pattern_b, 0xC3, sizeof(pattern_b)); + memcpy(image_a, pattern_a, sizeof(pattern_a)); + memcpy(image_b, pattern_b, sizeof(pattern_b)); + + void *placed = reserve_free_address(ALLOC_SIZE); + if (!placed) { + printf("SKIP aliased_images_survive_placed_map: no free address\n"); + return 0; + } + + uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + if (mem_map != placed) { + printf("FAIL aliased_images_survive_placed_map: placed map returned " + "%p, requested %p\n", + (void *)mem_map, placed); + return 1; + } + + agx_bo_unmap(&m.bo); + + int rc = 0; + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, image_a, CANARY_LEN) || + memcmp(readback, pattern_a, CANARY_LEN) != 0) { + printf("FAIL aliased_images_survive_placed_map: first image's retained " + "mapping is dangling after placed map + unmap\n"); + rc = 1; + } + if (!guarded_copy(readback, image_b, CANARY_LEN) || + memcmp(readback, pattern_b, CANARY_LEN) != 0) { + printf("FAIL aliased_images_survive_placed_map: second image's retained " + "mapping is dangling after placed map + unmap\n"); + rc = 1; + } + if (!rc) + printf("PASS aliased_images_survive_placed_map\n"); + return rc; +} + +int +main(void) +{ + int failures = 0; + + failures += placed_map_after_bind_then_unmap(); + failures += reserve_unmap_after_bind(); + failures += aliased_images_survive_placed_map(); + + if (failures) { + printf("%d case(s) failed: an application placed map or unmap destroys " + "the internal mapping retained by a bound image\n", + failures); + return 1; + } + return 0; +} From 1f824975aa5c38ed4e4898f89d6410d264bdb453 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:05:16 -0500 Subject: [PATCH 05/19] honeykrisp: model shared BO mappings in the placed-map regression Review follow-up on the CPU fixture: back the BO with one tmpfile (ftruncate to the BO size) and hand out MAP_SHARED mappings, so the internal image mapping, the application mapping and the placed mapping are separate mappings of the same BO contents. The fixture now checks coherence in both directions (retained vs earlier application mapping, placed mapping vs bind-offset contents) in addition to lifetime after unmap. The placed target keeps its PROT_NONE reservation mapped until the MAP_FIXED placed map replaces it. Every resource failure now FAILs the case; the SKIP paths are gone. The shim tracks and cleans up all mappings and the backing file per case. A map-before-bind control case covers the application-map-then-bind order that the proposed fix's stash logic must handle. Case bodies collapse into one data-driven runner; no new framework. --- src/asahi/lib/tests/test-placed-map.c | 338 ++++++++++++++------------ 1 file changed, 184 insertions(+), 154 deletions(-) diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c index db878a1798d2..4a49ef88478e 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/lib/tests/test-placed-map.c @@ -21,9 +21,13 @@ * This test drives the real production mapping code: agx_bo_map, * agx_bo_map_placed and agx_bo_unmap exactly as defined in agx_device.h, with * the exact call expressions used by hk_image_plane_bind, hk_MapMemory2KHR and - * hk_UnmapMemory2KHR. The only mock is the kernel mmap: mock_bo_mmap hands - * back anonymous pages for the request, like the DRM GEM mmap path does. There - * is no GPU, no device node and no Vulkan entrypoint; those are covered by the + * hk_UnmapMemory2KHR. The only mock is the kernel mmap: mock_bo_mmap backs the + * BO with one tmpfile (ftruncate to the BO size) and hands out MAP_SHARED + * mappings of it, so two live mappings of the same BO show the same bytes, + * like two CPU mappings of one GEM object. The placed target is a PROT_NONE + * reservation that stays mapped until the MAP_FIXED placed map replaces it. + * Every resource failure FAILs the case; there are no SKIP paths. There is no + * GPU, no device node and no Vulkan entrypoint; those are covered by the * admitted device gate (dEQP-VK host-copy coverage with a placed map after * image bind), not here. * @@ -38,6 +42,7 @@ #include #include #include +#include #include "agx_device.h" @@ -48,41 +53,96 @@ #define BO_SIZE 0x104000 /* Two images (or planes) bound to one memory object at different offsets, as - * hk_BindImageMemory2 allows. */ + * hk_BindImageMemory2 allows. Both offsets sit inside ALLOC_SIZE so the + * placed mapping can be checked for the same contents. */ #define BIND_OFF_A 0x1000 #define BIND_OFF_B 0x80000 #define CANARY_LEN 0x1000 +/* Case selection bits. Plain unmap (hk_device_memory.c:405) is the default + * application teardown; CASE_RESERVE uses the :394 overlay instead. */ +#define CASE_RESERVE (1u << 0) +#define CASE_ALIAS (1u << 1) +#define CASE_MAP_FIRST (1u << 2) + +static const struct { + const char *name; + unsigned flags; +} cases[] = { + {"placed_map_after_bind_then_unmap", 0}, + {"reserve_unmap_after_bind", CASE_RESERVE}, + {"aliased_images_survive_placed_map", CASE_ALIAS}, + /* The proposed production fix stashes an existing application mapping + * around the internal map; this control exercises that order. */ + {"map_before_bind_control", CASE_MAP_FIRST}, +}; + +#define MOCK_MAX_MAPS 8 + +struct mock_map { + void *addr; + size_t len; +}; + struct mock_state { + /* Must stay first: ops callbacks only get the agx_device pointer. */ struct agx_device dev; struct agx_bo bo; + FILE *file; + struct mock_map maps[MOCK_MAX_MAPS]; + unsigned map_count; }; -/* The single mock: the kernel side of a GEM mmap. Body mirrors agx_bo_mmap - * (agx_device.c) minus the ioctls. */ +static void +mock_track(struct mock_state *m, void *addr, size_t len) +{ + for (unsigned i = 0; i < m->map_count; i++) { + if (m->maps[i].addr == addr) { + m->maps[i].len = len; + return; + } + } + if (m->map_count == MOCK_MAX_MAPS) { + fprintf(stderr, "mock: mapping table full\n"); + exit(99); + } + m->maps[m->map_count].addr = addr; + m->maps[m->map_count].len = len; + m->map_count++; +} + +/* The single mock: the kernel side of a GEM mmap. One tmpfile is the BO + * storage; every mapping is MAP_SHARED over it, at fixed_addr with MAP_FIXED + * when requested. Body mirrors agx_bo_mmap (agx_device.c) minus the ioctls. + * On mmap failure this clears _map/_map_size, which the production code + * already treats as the map-failure result. */ static void mock_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, size_t size) { - (void)dev; + struct mock_state *m = (struct mock_state *)dev; + if (bo->_map != NULL) { fprintf(stderr, "mock: bo double mapped\n"); exit(99); } - bo->_map = mmap(fixed_addr, size, PROT_READ | PROT_WRITE, - MAP_SHARED | MAP_ANONYMOUS | (fixed_addr ? MAP_FIXED : 0), - -1, 0); - if (bo->_map == MAP_FAILED) { + void *p = mmap(fixed_addr, size, PROT_READ | PROT_WRITE, + MAP_SHARED | (fixed_addr ? MAP_FIXED : 0), fileno(m->file), + 0); + if (p == MAP_FAILED) { bo->_map = NULL; bo->_map_size = 0; - } else { - bo->_map_size = size; + return; } + + bo->_map = p; + bo->_map_size = size; + mock_track(m, p, size); } -static void +static bool mock_bo_init(struct mock_state *m, size_t bo_size) { memset(m, 0, sizeof(*m)); @@ -90,17 +150,37 @@ mock_bo_init(struct mock_state *m, size_t bo_size) m->bo.dev = &m->dev; m->bo.size = bo_size; m->bo.refcnt = 1; + + m->file = tmpfile(); + if (!m->file) + return false; + if (ftruncate(fileno(m->file), (off_t)bo_size) != 0) { + fclose(m->file); + m->file = NULL; + return false; + } + return true; } -/* Grab a page-aligned address that is (almost certainly) free, so the placed - * map target cannot alias the retained mapping. */ +/* Per-case cleanup: drop every mapping the shim made and the backing file. */ +static void +mock_bo_cleanup(struct mock_state *m) +{ + for (unsigned i = 0; i < m->map_count; i++) + munmap(m->maps[i].addr, m->maps[i].len); + if (m->file) + fclose(m->file); +} + +/* Reserve the placed target with PROT_NONE and keep it mapped: the MAP_FIXED + * placed map replaces it in place, never releasing the address first. */ static void * -reserve_free_address(size_t size) +reserve_placed_address(struct mock_state *m, size_t size) { void *p = mmap(NULL, size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (p == MAP_FAILED) return NULL; - munmap(p, size); + mock_track(m, p, size); return p; } @@ -132,157 +212,108 @@ guarded_copy(void *dst, const void *src, size_t n) return !faulted; } -/* Bind (hk_image.c:1333-1335), placed map (hk_device_memory.c:362), plain - * unmap (:405), host-copy read through the retained pointer (hk_image.c:1523). - */ +#define CASE_FAIL(...) \ + do { \ + printf("FAIL %s: ", name); \ + printf(__VA_ARGS__); \ + putchar('\n'); \ + goto out; \ + } while (0) + static int -placed_map_after_bind_then_unmap(void) +run_case(const char *name, unsigned flags) { struct mock_state m; - mock_bo_init(&m, BO_SIZE); - - uint8_t *image_map = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; - uint8_t pattern[CANARY_LEN]; - memset(pattern, 0x5A, sizeof(pattern)); - memcpy(image_map, pattern, sizeof(pattern)); - - void *placed = reserve_free_address(ALLOC_SIZE); - if (!placed) { - printf("SKIP placed_map_after_bind_then_unmap: no free address\n"); - return 0; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + /* Optional plain application map before bind, hk_device_memory.c:362 with + * a NULL fixed address. */ + uint8_t *app_map = NULL; + if (flags & CASE_MAP_FIRST) { + app_map = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + if (app_map == NULL) + CASE_FAIL("plain application map returned NULL"); } - uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); - if (mem_map != placed) { - printf("FAIL placed_map_after_bind_then_unmap: placed map returned %p, " - "requested %p\n", - (void *)mem_map, placed); - return 1; + /* Bind (hk_image.c:1333-1335): internal mapping retained by the image. */ + void *internal = agx_bo_map(&m.bo); + if (internal == NULL) + CASE_FAIL("internal map returned NULL"); + uint8_t *image_a = (uint8_t *)internal + BIND_OFF_A; + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(image_a, pat_a, sizeof(pat_a)); + + uint8_t *image_b = NULL; + if (flags & CASE_ALIAS) { + image_b = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_B; + memcpy(image_b, pat_b, sizeof(pat_b)); } - agx_bo_unmap(&m.bo); - - uint8_t readback[CANARY_LEN]; - if (!guarded_copy(readback, image_map, CANARY_LEN)) { - printf("FAIL placed_map_after_bind_then_unmap: host copy through the " - "retained image mapping faults after placed map + unmap " - "(omacom/mesa#8 r4418383484)\n"); - return 1; - } - if (memcmp(readback, pattern, CANARY_LEN) != 0) { - printf("FAIL placed_map_after_bind_then_unmap: retained image mapping " - "was destroyed or reused: canary mismatch\n"); - return 1; - } - - printf("PASS placed_map_after_bind_then_unmap\n"); - return 0; -} - -/* Same sequence, but the application unmaps with - * VK_MEMORY_UNMAP_RESERVE_BIT_EXT: the production path (hk_device_memory.c:394) - * overlays the BO mapping with PROT_NONE, which must not touch the internal - * image mapping either. */ -static int -reserve_unmap_after_bind(void) -{ - struct mock_state m; - mock_bo_init(&m, BO_SIZE); - - uint8_t *image_map = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; - uint8_t pattern[CANARY_LEN]; - memset(pattern, 0xA5, sizeof(pattern)); - memcpy(image_map, pattern, sizeof(pattern)); - - void *placed = reserve_free_address(ALLOC_SIZE); - if (!placed) { - printf("SKIP reserve_unmap_after_bind: no free address\n"); - return 0; - } + /* Coherence: the retained mapping and an earlier application mapping of + * the same BO must show the same bytes. */ + if (app_map != NULL && + memcmp(app_map + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("application and internal mappings disagree on BO contents"); + /* Placed map at the reserved address (hk_device_memory.c:362). */ + void *placed = reserve_placed_address(&m, ALLOC_SIZE); + if (placed == NULL) + CASE_FAIL("could not reserve the placed target address"); uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); if (mem_map != placed) { - printf("FAIL reserve_unmap_after_bind: placed map returned %p, " - "requested %p\n", - (void *)mem_map, placed); - return 1; + CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, + placed); } - /* Production reserve expression (hk_device_memory.c:395). */ - void *err = mmap(m.bo._map, agx_bo_map_len(&m.bo), PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) { - printf("SKIP reserve_unmap_after_bind: reserve mmap failed\n"); - return 0; + /* Placed/internal coherence: the placed mapping covers the allocation and + * must show the same BO bytes at the bound offsets. */ + if (memcmp(mem_map + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("placed mapping does not show the BO contents at the bind " + "offset"); + if ((flags & CASE_ALIAS) && + memcmp(mem_map + BIND_OFF_B, pat_b, CANARY_LEN) != 0) + CASE_FAIL("placed mapping does not show the BO contents at the alias " + "offset"); + + /* Application teardown. Reserve: production :394-396; else :405. */ + if (flags & CASE_RESERVE) { + void *err = mmap(m.bo._map, agx_bo_map_len(&m.bo), PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (err == MAP_FAILED) + CASE_FAIL("reserve overlay mmap failed"); + m.bo._map = NULL; + m.bo._map_size = 0; + } else { + agx_bo_unmap(&m.bo); } - m.bo._map = NULL; - m.bo._map_size = 0; + /* Lifetime: host-copy read through the retained pointers (hk_image.c:1523). + * The placed map and unmap must never touch the internal mapping. */ uint8_t readback[CANARY_LEN]; - if (!guarded_copy(readback, image_map, CANARY_LEN)) { - printf("FAIL reserve_unmap_after_bind: host copy through the retained " - "image mapping faults after placed map + reserve unmap\n"); - return 1; - } - if (memcmp(readback, pattern, CANARY_LEN) != 0) { - printf("FAIL reserve_unmap_after_bind: retained image mapping was " - "destroyed: canary mismatch\n"); - return 1; + if (!guarded_copy(readback, image_a, CANARY_LEN)) + CASE_FAIL("host copy through the retained image mapping faults after " + "placed map + unmap (omacom/mesa#8 r4418383484)"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("retained image mapping was destroyed or reused: canary " + "mismatch"); + if (flags & CASE_ALIAS) { + if (!guarded_copy(readback, image_b, CANARY_LEN)) + CASE_FAIL("aliased image's retained mapping faults after placed map " + "+ unmap"); + if (memcmp(readback, pat_b, CANARY_LEN) != 0) + CASE_FAIL("aliased image's retained mapping canary mismatch"); } - printf("PASS reserve_unmap_after_bind\n"); - return 0; -} - -/* Two images bound to one memory object: one placed map must not invalidate - * either retained mapping (hk_BindImageMemory2 binds each plane of an aliasing - * image from the same mem->bo). */ -static int -aliased_images_survive_placed_map(void) -{ - struct mock_state m; - mock_bo_init(&m, BO_SIZE); - - uint8_t *image_a = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_A; - uint8_t *image_b = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_B; - uint8_t pattern_a[CANARY_LEN], pattern_b[CANARY_LEN]; - memset(pattern_a, 0x5A, sizeof(pattern_a)); - memset(pattern_b, 0xC3, sizeof(pattern_b)); - memcpy(image_a, pattern_a, sizeof(pattern_a)); - memcpy(image_b, pattern_b, sizeof(pattern_b)); - - void *placed = reserve_free_address(ALLOC_SIZE); - if (!placed) { - printf("SKIP aliased_images_survive_placed_map: no free address\n"); - return 0; - } - - uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); - if (mem_map != placed) { - printf("FAIL aliased_images_survive_placed_map: placed map returned " - "%p, requested %p\n", - (void *)mem_map, placed); - return 1; - } + printf("PASS %s\n", name); + rc = 0; - agx_bo_unmap(&m.bo); - - int rc = 0; - uint8_t readback[CANARY_LEN]; - if (!guarded_copy(readback, image_a, CANARY_LEN) || - memcmp(readback, pattern_a, CANARY_LEN) != 0) { - printf("FAIL aliased_images_survive_placed_map: first image's retained " - "mapping is dangling after placed map + unmap\n"); - rc = 1; - } - if (!guarded_copy(readback, image_b, CANARY_LEN) || - memcmp(readback, pattern_b, CANARY_LEN) != 0) { - printf("FAIL aliased_images_survive_placed_map: second image's retained " - "mapping is dangling after placed map + unmap\n"); - rc = 1; - } - if (!rc) - printf("PASS aliased_images_survive_placed_map\n"); +out: + mock_bo_cleanup(&m); return rc; } @@ -291,9 +322,8 @@ main(void) { int failures = 0; - failures += placed_map_after_bind_then_unmap(); - failures += reserve_unmap_after_bind(); - failures += aliased_images_survive_placed_map(); + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) + failures += run_case(cases[i].name, cases[i].flags); if (failures) { printf("%d case(s) failed: an application placed map or unmap destroys " From c70e5372227417c8ebac4f218c013f8a378fc1a1 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:05:16 -0500 Subject: [PATCH 06/19] honeykrisp: keep internal BO mappings independent of application maps A bound image retains a BO mapping independently of VkDeviceMemory.map. Replacing that mapping for a placed application map left host-copy pointers dangling even with one BO reference. Give internal and application mappings separate slots. Return mmap results from the native and virtio primitives instead of swapping shared state; release both mappings with the BO and preserve placed-map size/error paths. The four CPU regression cases failed on fixture head 9c3850ff2a14b9c2d0e4144e458b78d209c1538f and now pass: plain unmap, reserve unmap, aliased images, and application-map-before-bind. They use real mapping helpers with shared file-backed kernel mmap emulation. Vulkan entrypoint/device coverage and full driver compilation remain separate qualification gates; no hardware readiness claim. --- src/asahi/lib/agx_bo.c | 2 ++ src/asahi/lib/agx_bo.h | 8 +++++++ src/asahi/lib/agx_device.c | 24 ++++++++++--------- src/asahi/lib/agx_device.h | 33 ++++++++++++++++++++------- src/asahi/lib/agx_device_virtio.c | 15 ++++++------ src/asahi/lib/tests/test-placed-map.c | 23 ++++++------------- 6 files changed, 62 insertions(+), 43 deletions(-) diff --git a/src/asahi/lib/agx_bo.c b/src/asahi/lib/agx_bo.c index e28bcc43aeca..756bd1e31a93 100644 --- a/src/asahi/lib/agx_bo.c +++ b/src/asahi/lib/agx_bo.c @@ -187,6 +187,8 @@ account_bo(struct label_stat *stat, struct agx_bo *bo) if (bo->_map != NULL) stat->mapped_B += bo->size; + if (bo->_imap != NULL) + stat->mapped_B += bo->size; } static void diff --git a/src/asahi/lib/agx_bo.h b/src/asahi/lib/agx_bo.h index 927929e7ec78..c061d66254b5 100644 --- a/src/asahi/lib/agx_bo.h +++ b/src/asahi/lib/agx_bo.h @@ -91,6 +91,14 @@ struct agx_bo { */ size_t _map_size; + /* Internal mapping for driver-owned retainers (image binds, pools, + * scratch, printf). Independent of the application mapping in _map: a + * placed map, unmap or reserve unmap never drops it. Lives until the BO + * is freed. + */ + void *_imap; + size_t _imap_size; + /* Process-local index */ uint32_t handle; diff --git a/src/asahi/lib/agx_device.c b/src/asahi/lib/agx_device.c index f64599ba1146..c74be292a193 100644 --- a/src/asahi/lib/agx_device.c +++ b/src/asahi/lib/agx_device.c @@ -87,6 +87,11 @@ agx_bo_free(struct agx_device *dev, struct agx_bo *bo) const uint64_t handle = bo->handle; agx_bo_unmap(bo); + if (bo->_imap) { + munmap(bo->_imap, bo->_imap_size ? bo->_imap_size : bo->size); + bo->_imap = NULL; + bo->_imap_size = 0; + } /* Free the VA. No need to unmap the BO or unbind the VA, as the kernel will * take care of that when we close it. @@ -226,12 +231,10 @@ agx_bo_alloc(struct agx_device *dev, size_t size, size_t align, return bo; } -static void +static void * agx_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, size_t size) { - assert(bo->_map == NULL && "not double mapped"); - struct drm_asahi_gem_mmap_offset gem_mmap_offset = {.handle = bo->uapi_handle}; int ret, flags; @@ -243,18 +246,17 @@ agx_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, } flags = MAP_SHARED | (fixed_addr ? MAP_FIXED : 0); - bo->_map = os_mmap(fixed_addr, size, PROT_READ | PROT_WRITE, flags, dev->fd, - gem_mmap_offset.offset); - if (bo->_map == MAP_FAILED) { + void *map = os_mmap(fixed_addr, size, PROT_READ | PROT_WRITE, flags, + dev->fd, gem_mmap_offset.offset); + if (map == MAP_FAILED) { fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i offset=0x%llx %m\n", - bo->_map, (long long)size, dev->fd, + map, (long long)size, dev->fd, (long long)gem_mmap_offset.offset); - bo->_map = NULL; - bo->_map_size = 0; - } else { - bo->_map_size = size; + return NULL; } + + return map; } struct agx_bo * diff --git a/src/asahi/lib/agx_device.h b/src/asahi/lib/agx_device.h index 10a331a10d05..9ff44870481d 100644 --- a/src/asahi/lib/agx_device.h +++ b/src/asahi/lib/agx_device.h @@ -77,8 +77,8 @@ typedef struct { enum agx_bo_flags flags); int (*bo_bind)(struct agx_device *dev, struct drm_asahi_gem_bind_op *ops, uint32_t count); - void (*bo_mmap)(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, - size_t size); + void *(*bo_mmap)(struct agx_device *dev, struct agx_bo *bo, + void *fixed_addr, size_t size); ssize_t (*get_params)(struct agx_device *dev, void *buf, size_t size); int (*submit)(struct agx_device *dev, struct drm_asahi_submit *submit, struct agx_submit_virt *virt); @@ -210,6 +210,8 @@ agx_bo_map_len(const struct agx_bo *bo) return bo->_map_size ? bo->_map_size : bo->size; } +/* Drop the application mapping. The internal mapping (_imap) is owned by the + * driver and is never touched by application map/unmap paths. */ static inline void agx_bo_unmap(struct agx_bo *bo) { @@ -220,9 +222,11 @@ agx_bo_unmap(struct agx_bo *bo) } } -/* Map the first `size` bytes of the BO, at fixed_addr when it is not NULL. A - * mapping left by an earlier owner of a cached BO is dropped first, as it - * cannot be at the requested address. +/* Map the first `size` bytes of the BO as the application mapping, at + * fixed_addr when it is not NULL. A mapping left by an earlier owner of a + * cached BO is dropped first, as it cannot be at the requested address. The + * internal mapping (_imap) is independent: dropping or replacing the + * application mapping never invalidates it. */ static inline void * agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) @@ -230,16 +234,29 @@ agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) if (fixed_addr) agx_bo_unmap(bo); - if (!bo->_map) - bo->dev->ops.bo_mmap(bo->dev, bo, fixed_addr, size); + if (!bo->_map) { + bo->_map = bo->dev->ops.bo_mmap(bo->dev, bo, fixed_addr, size); + if (bo->_map) + bo->_map_size = size; + } return bo->_map; } +/* Internal mapping, whole BO. Used by driver-owned retainers (image binds, + * pools, scratch, printf). Independent of the application mapping, so it + * stays valid across placed maps, vkUnmapMemory and reserve unmaps. Lives + * until the BO is freed. */ static inline void * agx_bo_map(struct agx_bo *bo) { - return agx_bo_map_placed(bo, NULL, bo->size); + if (!bo->_imap) { + bo->_imap = bo->dev->ops.bo_mmap(bo->dev, bo, NULL, bo->size); + if (bo->_imap) + bo->_imap_size = bo->size; + } + + return bo->_imap; } static inline bool diff --git a/src/asahi/lib/agx_device_virtio.c b/src/asahi/lib/agx_device_virtio.c index 488f4789654e..fa00e0a15be8 100644 --- a/src/asahi/lib/agx_device_virtio.c +++ b/src/asahi/lib/agx_device_virtio.c @@ -190,19 +190,18 @@ agx_virtio_bo_unbind_object(struct agx_device *dev, uint32_t object_handle) return 0; } -static void +static void * agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, size_t size) { - bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, size, fixed_addr); - if (bo->_map == MAP_FAILED) { - fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i\n", bo->_map, + void *map = vdrm_bo_map(dev->vdrm, bo->handle, size, fixed_addr); + if (map == MAP_FAILED) { + fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i\n", map, (long long)size, dev->fd); - bo->_map = NULL; - bo->_map_size = 0; - } else { - bo->_map_size = size; + return NULL; } + + return map; } static ssize_t diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c index 4a49ef88478e..63543f79347b 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/lib/tests/test-placed-map.c @@ -114,32 +114,23 @@ mock_track(struct mock_state *m, void *addr, size_t len) /* The single mock: the kernel side of a GEM mmap. One tmpfile is the BO * storage; every mapping is MAP_SHARED over it, at fixed_addr with MAP_FIXED - * when requested. Body mirrors agx_bo_mmap (agx_device.c) minus the ioctls. - * On mmap failure this clears _map/_map_size, which the production code - * already treats as the map-failure result. */ -static void + * when requested. Body mirrors agx_bo_mmap (agx_device.c) minus the ioctls; + * like the production primitive it only hands back a pointer (or NULL) and + * leaves the slot bookkeeping to the production inlines. */ +static void * mock_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, size_t size) { struct mock_state *m = (struct mock_state *)dev; - if (bo->_map != NULL) { - fprintf(stderr, "mock: bo double mapped\n"); - exit(99); - } - void *p = mmap(fixed_addr, size, PROT_READ | PROT_WRITE, MAP_SHARED | (fixed_addr ? MAP_FIXED : 0), fileno(m->file), 0); - if (p == MAP_FAILED) { - bo->_map = NULL; - bo->_map_size = 0; - return; - } + if (p == MAP_FAILED) + return NULL; - bo->_map = p; - bo->_map_size = size; mock_track(m, p, size); + return p; } static bool From 2a588f19baa6eecfe3e99a8a5cf215c4ccf7fe16 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:31:43 -0500 Subject: [PATCH 07/19] honeykrisp: cover stable BO mapping and shared-import lifetime in placed-map regression Follow-up findings on the independent-slots change: decode.c reads bo->_map directly (agxdecode_grab_mapped, fetch_gpu_mem, cmdstream) and wrap.c populates ._map at BO creation, so the stable BO mapping must keep existing and stay pointer-stable across application maps; and two memory objects sharing one imported BO must map independently, since an ordinary map currently returns another importer's placed mapping and that importer's unmap destroys it. Two new cases on the same tmpfile-backed shared kernel seam: the decoder-style stable mapping must match agx_bo_map, stay coherent with a placed application mapping and survive placed map + unmap with an unchanged pointer; and two importers (refcnt 2) must get distinct mappings with mutual coherence, where the first importer's unmap leaves the second's mapping valid. Application-map call sites of these cases adapt to the per-memory mapping shape in the production fix commit; assertions do not change. All resource failures FAIL; no SKIP paths. --- src/asahi/lib/tests/test-placed-map.c | 139 ++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c index 63543f79347b..0baf287edaf7 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/lib/tests/test-placed-map.c @@ -31,6 +31,15 @@ * admitted device gate (dEQP-VK host-copy coverage with a placed map after * image bind), not here. * + * Two further cases encode follow-up findings: the stable BO mapping that + * decode.c and wrap.c read and populate directly (_map) must survive the + * application map lifecycle, and two memory objects sharing one imported BO + * must map independently (an ordinary map must not reuse another importer's + * placed mapping, and one importer's unmap must not destroy the other's + * mapping). The application-map call sites of these two cases adapt to the + * per-memory mapping shape in the production fix commit; their assertions do + * not change. + * * Fails at head ddfb4cf114a6: the retained image mapping is destroyed and the * host-copy read faults or reads foreign memory. Passes once the internal * image mapping is independent of the application mapping. @@ -308,11 +317,141 @@ run_case(const char *name, unsigned flags) return rc; } +/* Follow-up finding: decode.c (agxdecode_grab_mapped :102, fetch :136, + * cmdstream :976) and wrap.c (:210, :230) read and populate bo->_map directly. + * The stable BO mapping must exist, match agx_bo_map, stay pointer-stable and + * coherent through the application map lifecycle. */ +static int +stable_map_survives_application_maps(void) +{ + const char *name = "stable_map_survives_application_maps"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + /* Decoder-style stable mapping. */ + uint8_t *stable = agx_bo_map(&m.bo); + if (stable == NULL) + CASE_FAIL("internal map returned NULL"); + if (m.bo._map == NULL) + CASE_FAIL("internal map did not populate the stable BO mapping _map " + "read by decode.c and wrap.c"); + if (m.bo._map != (void *)stable) + CASE_FAIL("agx_bo_map result does not match the stable BO mapping"); + memset(stable, 0x77, CANARY_LEN); + + /* Application placed map over the same BO (hk_device_memory.c:362). */ + void *placed = reserve_placed_address(&m, ALLOC_SIZE); + if (placed == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + if (mem_map != placed) { + CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, + placed); + } + if (memcmp(mem_map, stable, CANARY_LEN) != 0) + CASE_FAIL("placed mapping disagrees with the stable BO mapping"); + + /* Application unmap (hk_device_memory.c:405). */ + agx_bo_unmap(&m.bo); + + /* The stable mapping survives: same pointer, same bytes. */ + if (m.bo._map != (void *)stable) + CASE_FAIL("application maps replaced the stable BO mapping pointer"); + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, stable, CANARY_LEN)) + CASE_FAIL("stable BO mapping faults after application placed map + " + "unmap"); + for (size_t i = 0; i < CANARY_LEN; i++) { + if (readback[i] != 0x77) + CASE_FAIL("stable BO mapping was destroyed or reused: canary " + "mismatch at offset %zu", + i); + } + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +/* Follow-up finding: two VkDeviceMemory importers share one BO. An ordinary + * map of the second importer must not reuse the first importer's placed + * mapping, and the first importer's unmap must not destroy the second's + * mapping. Both mappings must show the same BO contents. */ +static int +shared_bo_importers_map_independently(void) +{ + const char *name = "shared_bo_importers_map_independently"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ + + /* Importer A: placed map (hk_device_memory.c:362, fixed address). */ + void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); + if (placed_a == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *map_a = agx_bo_map_placed(&m.bo, placed_a, ALLOC_SIZE); + if (map_a != placed_a) { + CASE_FAIL("importer A placed map returned %p, requested %p", + (void *)map_a, placed_a); + } + + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); + + /* Importer B: ordinary map (hk_device_memory.c:362, NULL fixed, whole + * BO). It must be its own mapping, not importer A's. */ + uint8_t *map_b = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + if (map_b == NULL) + CASE_FAIL("importer B map returned NULL"); + if (map_b == map_a) + CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + + /* Coherence: both mappings show the same BO contents. */ + if (memcmp(map_b + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("importer B mapping disagrees with importer A on BO " + "contents"); + memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); + if (memcmp(map_a + BIND_OFF_B, pat_b, CANARY_LEN) != 0) + CASE_FAIL("importer A mapping disagrees with importer B on BO " + "contents"); + + /* Importer A unmaps (hk_device_memory.c:405). Only A's mapping may die. */ + agx_bo_unmap(&m.bo); + + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) + CASE_FAIL("importer B mapping faults after importer A unmap"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("importer B mapping was destroyed or reused by importer A's " + "unmap: canary mismatch"); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + int main(void) { int failures = 0; + failures += stable_map_survives_application_maps(); + failures += shared_bo_importers_map_independently(); + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) failures += run_case(cases[i].name, cases[i].flags); From 30c3c8799780f2251f45cd0c02bd368561fbd57d Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:01:28 -0500 Subject: [PATCH 08/19] honeykrisp: cover the free path of shared imported BOs in placed-map regression The user-required free scenario: two memory objects share one imported BO and one is freed. The new case runs the BO-relevant hk_FreeMemory sequence for the freed object - the placed-unmap condition of hk_device_memory.c:283 and the real agx_bo_unreference refcount drop - and requires the survivor's mapping to stay valid and coherent, with the shared BO left at refcount 1. The heap accounting, ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a live vk_device and stay behind the admitted device gate. --- src/asahi/lib/tests/test-placed-map.c | 84 +++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c index 0baf287edaf7..e487e01bfb2b 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/lib/tests/test-placed-map.c @@ -40,6 +40,14 @@ * per-memory mapping shape in the production fix commit; their assertions do * not change. * + * A further case exercises the free path for the shared-BO scenario: freeing + * one of two memory objects sharing an imported BO runs the BO-relevant + * hk_FreeMemory sequence (the placed-unmap condition of hk_device_memory.c:283 + * and the real agx_bo_unreference refcount drop) and must leave the + * survivor's mapping valid. The heap accounting, ext-BO tracking and + * vk_device_memory_destroy steps of hk_FreeMemory need a live hk_device and + * stay behind the admitted device gate. + * * Fails at head ddfb4cf114a6: the retained image mapping is destroyed and the * host-copy read faults or reads foreign memory. Passes once the internal * image mapping is independent of the application mapping. @@ -444,6 +452,81 @@ shared_bo_importers_map_independently(void) return rc; } +/* The user-required free path: two memory objects share one imported BO; + * freeing one runs the BO-relevant hk_FreeMemory sequence (placed-unmap + * condition, hk_device_memory.c:283, plus the real agx_bo_unreference) and + * the survivor's mapping must stay valid and coherent. The heap accounting, + * ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a + * live vk_device and stay behind the admitted device gate. */ +static int +free_one_importer_survivor_survives(void) +{ + const char *name = "free_one_importer_survivor_survives"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ + + /* Importer A: placed map (hk_device_memory.c:362, fixed address). */ + void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); + if (placed_a == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *map_a = agx_bo_map_placed(&m.bo, placed_a, ALLOC_SIZE); + if (map_a != placed_a) { + CASE_FAIL("importer A placed map returned %p, requested %p", + (void *)map_a, placed_a); + } + + /* Importer B: ordinary map (hk_device_memory.c:362, NULL fixed). */ + uint8_t *map_b = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + if (map_b == NULL) + CASE_FAIL("importer B map returned NULL"); + if (map_b == map_a) + CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); + memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); + + /* vkFreeMemory(A): the BO-relevant hk_FreeMemory sequence. A holds a + * placed mapping, so the :283 condition fires for A's mapping, and the + * real refcount drop returns the BO alive at refcount 1 for B. */ + bool mem_a_mapped = true, mem_a_placed = true; + if (mem_a_mapped && mem_a_placed) + agx_bo_unmap(&m.bo); + agx_bo_unreference(&m.dev, &m.bo); + if (p_atomic_read(&m.bo.refcnt) != 1) + CASE_FAIL("freeing one importer did not leave the shared BO at " + "refcount 1 (got %d)", + p_atomic_read(&m.bo.refcnt)); + + /* Survivor B: mapping still valid, both canaries coherent. */ + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) + CASE_FAIL("survivor mapping faults after the other importer was " + "freed"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("survivor mapping was destroyed or reused by the other " + "importer's free: canary mismatch"); + if (!guarded_copy(readback, map_b + BIND_OFF_B, CANARY_LEN)) + CASE_FAIL("survivor mapping faults on its own written range after " + "the other importer was freed"); + if (memcmp(readback, pat_b, CANARY_LEN) != 0) + CASE_FAIL("survivor's own written range corrupted after the other " + "importer was freed"); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + int main(void) { @@ -451,6 +534,7 @@ main(void) failures += stable_map_survives_application_maps(); failures += shared_bo_importers_map_independently(); + failures += free_one_importer_survivor_survives(); for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) failures += run_case(cases[i].name, cases[i].flags); From 8531e06b696d1da58e304adfae1057dd5d6de9c6 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:09:24 -0500 Subject: [PATCH 09/19] honeykrisp: link the placed-map regression against libasahi_lib The free-path case calls the real agx_bo_unreference, which lives in agx_bo.c in libasahi_lib; the test target never linked the library, so the parent's red run failed at link with an undefined reference instead of exercising the refcount. Mirror the libasahi_tests dependency pattern: link_with libasahi_lib, which propagates its own dependency closure (dep_libdrm, idep_nir, libvdrm, libasahi_decode, idep_libagx, idep_libpoly) to the consumer. No test or assertion changed. --- src/asahi/lib/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/src/asahi/lib/meson.build b/src/asahi/lib/meson.build index 007ad4ec6285..7e21c40ff888 100644 --- a/src/asahi/lib/meson.build +++ b/src/asahi/lib/meson.build @@ -78,6 +78,7 @@ if with_tests '-I' + join_paths(meson.current_source_dir(), '..', 'libagx')], gnu_symbol_visibility : 'hidden', dependencies : [idep_mesautil, idep_agx_pack], + link_with : [libasahi_lib], include_directories : [inc_src, inc_asahi, inc_virtio_vdrm], ), suite : ['asahi'], From 6acf78cb044b307920606fd8acee487e447f2595 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:38:28 -0500 Subject: [PATCH 10/19] honeykrisp: give each memory object its own application mapping Parent-observed red at e14884a59caa (pr8-owner-red3.log): the stable BO mapping was absent because agx_bo_map populated only _imap, and two memory objects sharing an imported BO shared one replaceable mapping - importer B's ordinary map returned importer A's placed mapping, so freeing or unmapping A destroyed B's pointer. Restore bo->_map as the single stable driver-internal mapping and delete the _imap slot: agx_bo_map creates and returns _map (whole BO), decode.c and wrap.c keep reading and populating it directly, the BO cache keeps it as before, and agx_bo_free releases it. Application mappings leave the BO entirely: hk_MapMemory2KHR obtains a fresh mapping per memory object from the returning bo_mmap primitive (placed maps at the requested fixed address, ordinary maps whole-BO at a kernel-chosen address) into the existing mem->map plus a new mem->map_size; hk_UnmapMemory2KHR plain and reserve paths, the placed-address mismatch error path and vkFreeMemory release exactly that object's mapping. The shared-BO guard is removed rather than layered: with no replaceable mapping on the BO there is nothing left for it to protect, and two importers now map independently with coherence coming from the shared pages. Offset/size placed contract, mmap failure handling, the already-mapped check and error results are unchanged. The regression keeps its assertions and adapts only the application call expressions to the per-memory shape. Red lineage cited in the test header (8af285bd30c, e14884a59ca). No device run: hardware, CTS and precision gates hold. --- src/asahi/lib/agx_bo.c | 2 - src/asahi/lib/agx_bo.h | 18 ++- src/asahi/lib/agx_device.c | 5 - src/asahi/lib/agx_device.h | 39 ++----- src/asahi/lib/tests/test-placed-map.c | 151 ++++++++++++++------------ src/asahi/vulkan/hk_device_memory.c | 47 ++++---- src/asahi/vulkan/hk_device_memory.h | 9 +- 7 files changed, 125 insertions(+), 146 deletions(-) diff --git a/src/asahi/lib/agx_bo.c b/src/asahi/lib/agx_bo.c index 756bd1e31a93..e28bcc43aeca 100644 --- a/src/asahi/lib/agx_bo.c +++ b/src/asahi/lib/agx_bo.c @@ -187,8 +187,6 @@ account_bo(struct label_stat *stat, struct agx_bo *bo) if (bo->_map != NULL) stat->mapped_B += bo->size; - if (bo->_imap != NULL) - stat->mapped_B += bo->size; } static void diff --git a/src/asahi/lib/agx_bo.h b/src/asahi/lib/agx_bo.h index c061d66254b5..cca919e2e009 100644 --- a/src/asahi/lib/agx_bo.h +++ b/src/asahi/lib/agx_bo.h @@ -82,23 +82,19 @@ struct agx_bo { /* Mapping */ struct agx_va *va; - /* Suffixed to force agx_bo_map access */ + /* Suffixed to force agx_bo_map access. The stable driver-internal + * mapping: decode.c reads it directly and wrap.c populates it at BO + * creation. Application mappings do not live here; each VkDeviceMemory + * owns its own mapping, so an application placed map, unmap or reserve + * unmap never touches this one. Lives until the BO is freed. + */ void *_map; /* Length of the live mapping in _map. Zero when the mapping was not made - * by bo_mmap (the trace wrapper), which covers the whole BO. A placed - * mapping can be shorter than the BO. + * by bo_mmap (the trace wrapper), which covers the whole BO. */ size_t _map_size; - /* Internal mapping for driver-owned retainers (image binds, pools, - * scratch, printf). Independent of the application mapping in _map: a - * placed map, unmap or reserve unmap never drops it. Lives until the BO - * is freed. - */ - void *_imap; - size_t _imap_size; - /* Process-local index */ uint32_t handle; diff --git a/src/asahi/lib/agx_device.c b/src/asahi/lib/agx_device.c index c74be292a193..6c843156cfd1 100644 --- a/src/asahi/lib/agx_device.c +++ b/src/asahi/lib/agx_device.c @@ -87,11 +87,6 @@ agx_bo_free(struct agx_device *dev, struct agx_bo *bo) const uint64_t handle = bo->handle; agx_bo_unmap(bo); - if (bo->_imap) { - munmap(bo->_imap, bo->_imap_size ? bo->_imap_size : bo->size); - bo->_imap = NULL; - bo->_imap_size = 0; - } /* Free the VA. No need to unmap the BO or unbind the VA, as the kernel will * take care of that when we close it. diff --git a/src/asahi/lib/agx_device.h b/src/asahi/lib/agx_device.h index 9ff44870481d..c2e125026b1c 100644 --- a/src/asahi/lib/agx_device.h +++ b/src/asahi/lib/agx_device.h @@ -210,8 +210,8 @@ agx_bo_map_len(const struct agx_bo *bo) return bo->_map_size ? bo->_map_size : bo->size; } -/* Drop the application mapping. The internal mapping (_imap) is owned by the - * driver and is never touched by application map/unmap paths. */ +/* Drop the stable internal mapping. Application mappings are owned by their + * VkDeviceMemory and never pass through here. */ static inline void agx_bo_unmap(struct agx_bo *bo) { @@ -222,43 +222,22 @@ agx_bo_unmap(struct agx_bo *bo) } } -/* Map the first `size` bytes of the BO as the application mapping, at - * fixed_addr when it is not NULL. A mapping left by an earlier owner of a - * cached BO is dropped first, as it cannot be at the requested address. The - * internal mapping (_imap) is independent: dropping or replacing the - * application mapping never invalidates it. - */ +/* Stable internal mapping, whole BO. Used by driver-owned retainers (image + * binds, pools, scratch, printf) and read directly by the decoder. Stays + * valid across application placed maps, vkUnmapMemory and reserve unmaps. + * Lives until the BO is freed. */ static inline void * -agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) +agx_bo_map(struct agx_bo *bo) { - if (fixed_addr) - agx_bo_unmap(bo); - if (!bo->_map) { - bo->_map = bo->dev->ops.bo_mmap(bo->dev, bo, fixed_addr, size); + bo->_map = bo->dev->ops.bo_mmap(bo->dev, bo, NULL, bo->size); if (bo->_map) - bo->_map_size = size; + bo->_map_size = bo->size; } return bo->_map; } -/* Internal mapping, whole BO. Used by driver-owned retainers (image binds, - * pools, scratch, printf). Independent of the application mapping, so it - * stays valid across placed maps, vkUnmapMemory and reserve unmaps. Lives - * until the BO is freed. */ -static inline void * -agx_bo_map(struct agx_bo *bo) -{ - if (!bo->_imap) { - bo->_imap = bo->dev->ops.bo_mmap(bo->dev, bo, NULL, bo->size); - if (bo->_imap) - bo->_imap_size = bo->size; - } - - return bo->_imap; -} - static inline bool agx_has_soft_fault(struct agx_device *dev) { diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/lib/tests/test-placed-map.c index e487e01bfb2b..f4302db4b5b5 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/lib/tests/test-placed-map.c @@ -5,52 +5,49 @@ /* * CPU-only regression for the omacom/mesa#8 review finding - * (https://github.com/omacom/mesa/pull/8#discussion_r4418383484): + * (https://github.com/omacom/mesa/pull/8#discussion_r4418383484) and its + * follow-ups: * * A non-sparse image bind retains an internal host-copy mapping * (hk_image_plane_bind, hk_image.c:1334: plane->map = agx_bo_map(mem->bo) + - * offset) without setting mem->map or taking a BO reference. A subsequent - * application vkMapMemory2KHR with VK_MEMORY_MAP_PLACED_BIT_EXT reaches - * agx_bo_map_placed (agx_device.h) with a fixed address, which unconditionally - * drops the BO's current mapping. The retained plane->map then dangles, and - * the host-copy paths (hk_image.c:1444, :1523, :1610) read and write through - * it. The plain unmap path (hk_device_memory.c:405) and the - * VK_MEMORY_UNMAP_RESERVE_BIT_EXT path (:394) invalidate the same retained - * pointer. + * offset) without setting mem->map or taking a BO reference. While the + * application mapping lived on the BO, any vkMapMemory2KHR placed map, + * vkUnmapMemory2KHR (plain or VK_MEMORY_UNMAP_RESERVE_BIT_EXT) or + * vkFreeMemory of the same memory destroyed that retained pointer, and two + * memory objects sharing an imported BO shared one replaceable mapping. The + * fix gives the BO a single stable internal mapping (agx_bo_map -> + * bo->_map, read directly by decode.c and populated by wrap.c) and gives + * each VkDeviceMemory its own application mapping from the bo_mmap + * primitive, dropped only by that object's unmap, reserve unmap and free. * - * This test drives the real production mapping code: agx_bo_map, - * agx_bo_map_placed and agx_bo_unmap exactly as defined in agx_device.h, with - * the exact call expressions used by hk_image_plane_bind, hk_MapMemory2KHR and - * hk_UnmapMemory2KHR. The only mock is the kernel mmap: mock_bo_mmap backs the - * BO with one tmpfile (ftruncate to the BO size) and hands out MAP_SHARED - * mappings of it, so two live mappings of the same BO show the same bytes, - * like two CPU mappings of one GEM object. The placed target is a PROT_NONE - * reservation that stays mapped until the MAP_FIXED placed map replaces it. - * Every resource failure FAILs the case; there are no SKIP paths. There is no - * GPU, no device node and no Vulkan entrypoint; those are covered by the - * admitted device gate (dEQP-VK host-copy coverage with a placed map after - * image bind), not here. + * This test drives the real production mapping code: agx_bo_map exactly as + * defined in agx_device.h with the bind expression of hk_image_plane_bind, + * the per-memory primitive expressions of hk_MapMemory2KHR, the per-memory + * teardown of hk_UnmapMemory2KHR and hk_FreeMemory, and the real + * agx_bo_unreference. The only mock is the kernel mmap: mock_bo_mmap backs + * the BO with one tmpfile (ftruncate to the BO size) and hands out + * MAP_SHARED mappings of it, so two live mappings of the same BO show the + * same bytes, like two CPU mappings of one GEM object. The placed target is + * a PROT_NONE reservation that stays mapped until the MAP_FIXED placed map + * replaces it. Every resource failure FAILs the case; there are no SKIP + * paths. There is no GPU, no device node and no Vulkan entrypoint; those + * are covered by the admitted device gate (dEQP-VK host-copy coverage with + * a placed map after image bind), not here. * - * Two further cases encode follow-up findings: the stable BO mapping that - * decode.c and wrap.c read and populate directly (_map) must survive the - * application map lifecycle, and two memory objects sharing one imported BO - * must map independently (an ordinary map must not reuse another importer's - * placed mapping, and one importer's unmap must not destroy the other's - * mapping). The application-map call sites of these two cases adapt to the - * per-memory mapping shape in the production fix commit; their assertions do - * not change. + * The stable-map case pins the decoder contract: the internal mapping must + * exist, match agx_bo_map, stay pointer-stable and coherent through the + * application map lifecycle. The two-importer and free cases pin shared-BO + * lifetime: distinct mappings per memory object, mutual content coherence, + * and a survivor whose mapping outlives the other importer's unmap or + * free. The heap accounting, ext-BO tracking and vk_device_memory_destroy + * tail of hk_FreeMemory need a live vk_device and stay behind the admitted + * device gate. * - * A further case exercises the free path for the shared-BO scenario: freeing - * one of two memory objects sharing an imported BO runs the BO-relevant - * hk_FreeMemory sequence (the placed-unmap condition of hk_device_memory.c:283 - * and the real agx_bo_unreference refcount drop) and must leave the - * survivor's mapping valid. The heap accounting, ext-BO tracking and - * vk_device_memory_destroy steps of hk_FreeMemory need a live hk_device and - * stay behind the admitted device gate. - * - * Fails at head ddfb4cf114a6: the retained image mapping is destroyed and the - * host-copy read faults or reads foreign memory. Passes once the internal - * image mapping is independent of the application mapping. + * Red was parent-observed on the pre-fix lineage (fixture heads 8af285bd30c + * and e14884a59ca: stable _map absent, importer B reusing importer A's + * placed mapping; the free case stopped at the same pointer-independence + * assertion before reaching the survivor checks). Passes with per-memory + * ownership. */ #include @@ -237,11 +234,12 @@ run_case(const char *name, unsigned flags) if (!mock_bo_init(&m, BO_SIZE)) CASE_FAIL("tmpfile/ftruncate for the BO failed"); - /* Optional plain application map before bind, hk_device_memory.c:362 with - * a NULL fixed address. */ + /* Optional plain application map before bind: a mapping owned by its own + * memory object (the hk_MapMemory2KHR ordinary expression: a fresh + * mapping from the bo_mmap primitive, never stored on the BO). */ uint8_t *app_map = NULL; if (flags & CASE_MAP_FIRST) { - app_map = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + app_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); if (app_map == NULL) CASE_FAIL("plain application map returned NULL"); } @@ -268,11 +266,13 @@ run_case(const char *name, unsigned flags) memcmp(app_map + BIND_OFF_A, pat_a, CANARY_LEN) != 0) CASE_FAIL("application and internal mappings disagree on BO contents"); - /* Placed map at the reserved address (hk_device_memory.c:362). */ + /* Placed map at the reserved address: a mapping owned by its own memory + * object (the hk_MapMemory2KHR placed expression: the primitive at the + * requested fixed address). */ void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + uint8_t *mem_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); @@ -288,16 +288,18 @@ run_case(const char *name, unsigned flags) CASE_FAIL("placed mapping does not show the BO contents at the alias " "offset"); - /* Application teardown. Reserve: production :394-396; else :405. */ + /* Application teardown, owned by the mapped memory object + * (hk_UnmapMemory2KHR): reserve overlays exactly that mapping, a plain + * unmap releases it. The stable BO mapping is never touched. */ if (flags & CASE_RESERVE) { - void *err = mmap(m.bo._map, agx_bo_map_len(&m.bo), PROT_NONE, + void *err = mmap(mem_map, ALLOC_SIZE, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); if (err == MAP_FAILED) CASE_FAIL("reserve overlay mmap failed"); - m.bo._map = NULL; - m.bo._map_size = 0; + mem_map = NULL; } else { - agx_bo_unmap(&m.bo); + munmap(mem_map, ALLOC_SIZE); + mem_map = NULL; } /* Lifetime: host-copy read through the retained pointers (hk_image.c:1523). @@ -350,11 +352,12 @@ stable_map_survives_application_maps(void) CASE_FAIL("agx_bo_map result does not match the stable BO mapping"); memset(stable, 0x77, CANARY_LEN); - /* Application placed map over the same BO (hk_device_memory.c:362). */ + /* Application placed map over the same BO: a mapping owned by its own + * memory object (the hk_MapMemory2KHR placed expression). */ void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = agx_bo_map_placed(&m.bo, placed, ALLOC_SIZE); + uint8_t *mem_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); @@ -362,8 +365,10 @@ stable_map_survives_application_maps(void) if (memcmp(mem_map, stable, CANARY_LEN) != 0) CASE_FAIL("placed mapping disagrees with the stable BO mapping"); - /* Application unmap (hk_device_memory.c:405). */ - agx_bo_unmap(&m.bo); + /* Application unmap of that memory object's mapping + * (hk_UnmapMemory2KHR). */ + munmap(mem_map, ALLOC_SIZE); + mem_map = NULL; /* The stable mapping survives: same pointer, same bytes. */ if (m.bo._map != (void *)stable) @@ -402,11 +407,12 @@ shared_bo_importers_map_independently(void) CASE_FAIL("tmpfile/ftruncate for the BO failed"); m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ - /* Importer A: placed map (hk_device_memory.c:362, fixed address). */ + /* Importer A: placed map (hk_MapMemory2KHR placed expression, fixed + * address). */ void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = agx_bo_map_placed(&m.bo, placed_a, ALLOC_SIZE); + uint8_t *map_a = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed_a, ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", (void *)map_a, placed_a); @@ -417,9 +423,9 @@ shared_bo_importers_map_independently(void) memset(pat_b, 0xC3, sizeof(pat_b)); memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); - /* Importer B: ordinary map (hk_device_memory.c:362, NULL fixed, whole - * BO). It must be its own mapping, not importer A's. */ - uint8_t *map_b = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression, NULL + * fixed, whole BO). It must be its own mapping, not importer A's. */ + uint8_t *map_b = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) @@ -434,8 +440,10 @@ shared_bo_importers_map_independently(void) CASE_FAIL("importer A mapping disagrees with importer B on BO " "contents"); - /* Importer A unmaps (hk_device_memory.c:405). Only A's mapping may die. */ - agx_bo_unmap(&m.bo); + /* Importer A unmaps its own mapping (hk_UnmapMemory2KHR). Only A's + * mapping may die. */ + munmap(map_a, ALLOC_SIZE); + map_a = NULL; uint8_t readback[CANARY_LEN]; if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) @@ -469,18 +477,20 @@ free_one_importer_survivor_survives(void) CASE_FAIL("tmpfile/ftruncate for the BO failed"); m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ - /* Importer A: placed map (hk_device_memory.c:362, fixed address). */ + /* Importer A: placed map (hk_MapMemory2KHR placed expression, fixed + * address). */ void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = agx_bo_map_placed(&m.bo, placed_a, ALLOC_SIZE); + uint8_t *map_a = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed_a, ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", (void *)map_a, placed_a); } - /* Importer B: ordinary map (hk_device_memory.c:362, NULL fixed). */ - uint8_t *map_b = agx_bo_map_placed(&m.bo, NULL, m.bo.size); + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression, NULL + * fixed). */ + uint8_t *map_b = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) @@ -492,12 +502,11 @@ free_one_importer_survivor_survives(void) memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); - /* vkFreeMemory(A): the BO-relevant hk_FreeMemory sequence. A holds a - * placed mapping, so the :283 condition fires for A's mapping, and the - * real refcount drop returns the BO alive at refcount 1 for B. */ - bool mem_a_mapped = true, mem_a_placed = true; - if (mem_a_mapped && mem_a_placed) - agx_bo_unmap(&m.bo); + /* vkFreeMemory(A): the mapping belongs to the freed memory object, so + * hk_FreeMemory drops exactly A's mapping, and the real refcount drop + * returns the BO alive at refcount 1 for B. */ + munmap(map_a, ALLOC_SIZE); + map_a = NULL; agx_bo_unreference(&m.dev, &m.bo); if (p_atomic_read(&m.bo.refcnt) != 1) CASE_FAIL("freeing one importer did not leave the shared BO at " diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index 679c575217dd..0cc37c57bd90 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -277,11 +277,14 @@ hk_FreeMemory(VkDevice device, VkDeviceMemory _mem, if (mem->bo->flags & (AGX_BO_SHAREABLE | AGX_BO_SHARED)) hk_remove_ext_bo(dev, mem->bo); - /* Do not hand a placed mapping, which lies in application address space, - * to the BO cache with the BO. + /* The mapping belongs to this memory object: drop it on free instead of + * handing it to the BO cache. */ - if (mem->map && mem->placed_map) - agx_bo_unmap(mem->bo); + if (mem->map) { + munmap(mem->map, mem->map_size); + mem->map = NULL; + mem->map_size = 0; + } agx_bo_unreference(&dev->dev, mem->bo); @@ -345,34 +348,29 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "A placed map must cover the whole allocation."); } - - /* An imported BO can be shared by two memory objects. Do not tear down - * a mapping the other one may hold. - */ - if (mem->bo->_map && p_atomic_read(&mem->bo->refcnt) > 1) { - return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Memory is shared and already mapped."); - } } /* A placed mapping covers exactly the allocation: the BO can be larger - * (BO cache, GPU page rounding) and the memory after the allocation belongs - * to the application. An ordinary mapping keeps the whole BO. + * (BO cache, GPU page rounding) and the memory after the allocation + * belongs to the application. An ordinary mapping keeps the whole BO. The + * mapping belongs to this memory object and is never stored on the BO, so + * memory objects sharing an imported BO map independently. */ - mem->map = agx_bo_map_placed(mem->bo, fixed_addr, - fixed_addr ? mem->vk.size : mem->bo->size); + size_t map_size = fixed_addr ? mem->vk.size : mem->bo->size; + mem->map = dev->dev.ops.bo_mmap(&dev->dev, mem->bo, fixed_addr, map_size); if (mem->map == NULL) { return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Failed to map memory object."); } if (fixed_addr && mem->map != fixed_addr) { - agx_bo_unmap(mem->bo); + munmap(mem->map, map_size); mem->map = NULL; return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Mapped address does not match the placed address."); } + mem->map_size = map_size; mem->placed_map = fixed_addr != NULL; *ppData = mem->map + offset; @@ -391,21 +389,22 @@ hk_UnmapMemory2KHR(VkDevice device, if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { VK_FROM_HANDLE(hk_device, dev, device); - if (mem->bo->_map) { - void *err = mmap(mem->bo->_map, agx_bo_map_len(mem->bo), PROT_NONE, + if (mem->map) { + void *err = mmap(mem->map, mem->map_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); if (err == MAP_FAILED) { return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Failed to map over original mapping"); } - mem->bo->_map = NULL; - mem->bo->_map_size = 0; + mem->map = NULL; + mem->map_size = 0; } - } else { - agx_bo_unmap(mem->bo); + } else if (mem->map) { + munmap(mem->map, mem->map_size); + mem->map = NULL; + mem->map_size = 0; } - mem->map = NULL; mem->placed_map = false; return VK_SUCCESS; diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index 6a29324442d9..91e5b46a955a 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -23,9 +23,12 @@ struct hk_device_memory { void *map; - /* The live mapping was made with VK_MEMORY_MAP_PLACED_BIT_EXT. Such a - * mapping belongs to this object, so it is dropped on unmap and on free - * instead of staying on a BO that returns to the BO cache. + /* Length of the live mapping in map, valid when map is not NULL. */ + size_t map_size; + + /* The live mapping was made with VK_MEMORY_MAP_PLACED_BIT_EXT. The + * mapping belongs to this object: it is dropped on unmap and on free, + * independently of any other memory object sharing the BO. */ bool placed_map; }; From 4e269c57f09a926a4e785c5e48788325c8b36846 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:02:47 -0500 Subject: [PATCH 11/19] honeykrisp: drop redundant mapping state The stable BO mapping always covers the whole BO. Its length is already bo->size. Application mapping lengths belong to each memory object, and the old placed-map flag has no readers after the ownership fix. --- src/asahi/lib/agx_bo.h | 5 ----- src/asahi/lib/agx_device.h | 11 +---------- src/asahi/vulkan/hk_device_memory.c | 3 --- src/asahi/vulkan/hk_device_memory.h | 6 +----- 4 files changed, 2 insertions(+), 23 deletions(-) diff --git a/src/asahi/lib/agx_bo.h b/src/asahi/lib/agx_bo.h index cca919e2e009..a367eaac60a7 100644 --- a/src/asahi/lib/agx_bo.h +++ b/src/asahi/lib/agx_bo.h @@ -90,11 +90,6 @@ struct agx_bo { */ void *_map; - /* Length of the live mapping in _map. Zero when the mapping was not made - * by bo_mmap (the trace wrapper), which covers the whole BO. - */ - size_t _map_size; - /* Process-local index */ uint32_t handle; diff --git a/src/asahi/lib/agx_device.h b/src/asahi/lib/agx_device.h index c2e125026b1c..dda0bc37055f 100644 --- a/src/asahi/lib/agx_device.h +++ b/src/asahi/lib/agx_device.h @@ -204,21 +204,14 @@ agx_rw_addr_to_ro(struct agx_device *dev, uint64_t addr) return addr + dev->sparse_ro_offset; } -static inline size_t -agx_bo_map_len(const struct agx_bo *bo) -{ - return bo->_map_size ? bo->_map_size : bo->size; -} - /* Drop the stable internal mapping. Application mappings are owned by their * VkDeviceMemory and never pass through here. */ static inline void agx_bo_unmap(struct agx_bo *bo) { if (bo->_map) { - munmap(bo->_map, agx_bo_map_len(bo)); + munmap(bo->_map, bo->size); bo->_map = NULL; - bo->_map_size = 0; } } @@ -231,8 +224,6 @@ agx_bo_map(struct agx_bo *bo) { if (!bo->_map) { bo->_map = bo->dev->ops.bo_mmap(bo->dev, bo, NULL, bo->size); - if (bo->_map) - bo->_map_size = bo->size; } return bo->_map; diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index 0cc37c57bd90..e46957d1b931 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -371,7 +371,6 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, } mem->map_size = map_size; - mem->placed_map = fixed_addr != NULL; *ppData = mem->map + offset; return VK_SUCCESS; @@ -405,8 +404,6 @@ hk_UnmapMemory2KHR(VkDevice device, mem->map_size = 0; } - mem->placed_map = false; - return VK_SUCCESS; } diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index 91e5b46a955a..1b6a18a2d688 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -21,16 +21,12 @@ struct hk_device_memory { struct agx_bo *bo; + /* Application mapping owned only by this memory object. */ void *map; /* Length of the live mapping in map, valid when map is not NULL. */ size_t map_size; - /* The live mapping was made with VK_MEMORY_MAP_PLACED_BIT_EXT. The - * mapping belongs to this object: it is dropped on unmap and on free, - * independently of any other memory object sharing the BO. - */ - bool placed_map; }; VK_DEFINE_NONDISP_HANDLE_CASTS(hk_device_memory, vk.base, VkDeviceMemory, From 8eec298f16b5ec7e8aa116e12dc83e78950d2159 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Thu, 29 Oct 2026 23:46:15 -0500 Subject: [PATCH 12/19] honeykrisp: share per-memory mapping helpers with the regression Move the map, unmap and reserve-unmap operations into shared helpers so the regression executes the same ownership paths as the Vulkan callers. Register the test with the Vulkan dependency bundle instead of copying its transitive headers. Proof: eight CPU mapping-lifetime cases pass and the full local Vulkan driver compiles. The earlier missing libagx header build failure is retained. No device qualification or merge approval is claimed. --- src/asahi/lib/meson.build | 14 -- src/asahi/vulkan/hk_device_memory.c | 35 +--- src/asahi/vulkan/hk_device_memory.h | 52 +++++ src/asahi/vulkan/meson.build | 20 ++ .../{lib => vulkan}/tests/test-placed-map.c | 189 +++++++++++++----- 5 files changed, 222 insertions(+), 88 deletions(-) rename src/asahi/{lib => vulkan}/tests/test-placed-map.c (75%) diff --git a/src/asahi/lib/meson.build b/src/asahi/lib/meson.build index 7e21c40ff888..e50a70eff518 100644 --- a/src/asahi/lib/meson.build +++ b/src/asahi/lib/meson.build @@ -69,20 +69,6 @@ libasahi_lib = static_library( ) if with_tests - test( - 'libasahi_placed_map_test', - executable( - 'libasahi_placed_map_test', - 'tests/test-placed-map.c', - c_args : [c_msvc_compat_args, no_override_init_args, - '-I' + join_paths(meson.current_source_dir(), '..', 'libagx')], - gnu_symbol_visibility : 'hidden', - dependencies : [idep_mesautil, idep_agx_pack], - link_with : [libasahi_lib], - include_directories : [inc_src, inc_asahi, inc_virtio_vdrm], - ), - suite : ['asahi'], - ) test( 'libasahi_tests', executable( diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index e46957d1b931..f7d9094de500 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -280,11 +280,7 @@ hk_FreeMemory(VkDevice device, VkDeviceMemory _mem, /* The mapping belongs to this memory object: drop it on free instead of * handing it to the BO cache. */ - if (mem->map) { - munmap(mem->map, mem->map_size); - mem->map = NULL; - mem->map_size = 0; - } + hk_memory_unmap(mem); agx_bo_unreference(&dev->dev, mem->bo); @@ -352,25 +348,20 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, /* A placed mapping covers exactly the allocation: the BO can be larger * (BO cache, GPU page rounding) and the memory after the allocation - * belongs to the application. An ordinary mapping keeps the whole BO. The - * mapping belongs to this memory object and is never stored on the BO, so - * memory objects sharing an imported BO map independently. + * belongs to the application. An ordinary mapping keeps the whole BO. */ size_t map_size = fixed_addr ? mem->vk.size : mem->bo->size; - mem->map = dev->dev.ops.bo_mmap(&dev->dev, mem->bo, fixed_addr, map_size); - if (mem->map == NULL) { + if (hk_memory_map_from_bo(&dev->dev, mem, fixed_addr, map_size) == NULL) { return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Failed to map memory object."); } if (fixed_addr && mem->map != fixed_addr) { - munmap(mem->map, map_size); - mem->map = NULL; + hk_memory_unmap(mem); return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Mapped address does not match the placed address."); } - mem->map_size = map_size; *ppData = mem->map + offset; return VK_SUCCESS; @@ -388,20 +379,12 @@ hk_UnmapMemory2KHR(VkDevice device, if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { VK_FROM_HANDLE(hk_device, dev, device); - if (mem->map) { - void *err = mmap(mem->map, mem->map_size, PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) { - return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to map over original mapping"); - } - mem->map = NULL; - mem->map_size = 0; + if (!hk_memory_unmap_reserve(mem)) { + return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, + "Failed to map over original mapping"); } - } else if (mem->map) { - munmap(mem->map, mem->map_size); - mem->map = NULL; - mem->map_size = 0; + } else { + hk_memory_unmap(mem); } return VK_SUCCESS; diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index 1b6a18a2d688..d33ec3623d21 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -9,10 +9,14 @@ #include "hk_private.h" +#include "asahi/lib/agx_device.h" + #include "vk_device_memory.h" #include "util/list.h" +#include + struct hk_device; struct hk_image_plane; @@ -29,6 +33,54 @@ struct hk_device_memory { }; +/* Map this memory object's own application mapping from the BO: a placed + * mapping covers exactly `size` bytes at `fixed_addr`, an ordinary mapping + * covers the whole BO. The mapping is never stored on the BO, so memory + * objects sharing an imported BO map independently. Returns the mapping, or + * NULL when the primitive failed. */ +static inline void * +hk_memory_map_from_bo(struct agx_device *dev, struct hk_device_memory *mem, + void *fixed_addr, size_t size) +{ + mem->map = dev->ops.bo_mmap(dev, mem->bo, fixed_addr, size); + if (mem->map) + mem->map_size = size; + + return mem->map; +} + +/* Drop this memory object's own application mapping. Other memory objects + * sharing the BO keep theirs, and the BO's stable internal mapping is never + * touched. A never-mapped object is a no-op. */ +static inline void +hk_memory_unmap(struct hk_device_memory *mem) +{ + if (mem->map) { + munmap(mem->map, mem->map_size); + mem->map = NULL; + mem->map_size = 0; + } +} + +/* VK_MEMORY_UNMAP_RESERVE_BIT_EXT: overlay this memory object's own mapping + * with PROT_NONE instead of releasing the address. Returns false when the + * overlay fails, leaving the mapping recorded. */ +static inline bool +hk_memory_unmap_reserve(struct hk_device_memory *mem) +{ + if (!mem->map) + return true; + + void *err = mmap(mem->map, mem->map_size, PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (err == MAP_FAILED) + return false; + + mem->map = NULL; + mem->map_size = 0; + return true; +} + VK_DEFINE_NONDISP_HANDLE_CASTS(hk_device_memory, vk.base, VkDeviceMemory, VK_OBJECT_TYPE_DEVICE_MEMORY) diff --git a/src/asahi/vulkan/meson.build b/src/asahi/vulkan/meson.build index dbe175d91d13..5b9276675b7c 100644 --- a/src/asahi/vulkan/meson.build +++ b/src/asahi/vulkan/meson.build @@ -150,3 +150,23 @@ _dev_icd = custom_target( ) devenv.append('VK_DRIVER_FILES', _dev_icd.full_path()) + +if with_tests + test( + 'libasahi_placed_map_test', + executable( + 'libasahi_placed_map_test', + 'tests/test-placed-map.c', + c_args : [c_msvc_compat_args, no_override_init_args, + '-Wno-c2x-extensions'], + gnu_symbol_visibility : 'hidden', + include_directories : [ + inc_gallium, inc_gallium_aux, inc_include, inc_src, + inc_asahi, inc_virtio_gpu, inc_virtio_vdrm, + ], + dependencies : [hk_deps, idep_mesautil], + link_with : [libasahi_lib], + ), + suite : ['asahi'], + ) +endif diff --git a/src/asahi/lib/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c similarity index 75% rename from src/asahi/lib/tests/test-placed-map.c rename to src/asahi/vulkan/tests/test-placed-map.c index f4302db4b5b5..47816ffa528f 100644 --- a/src/asahi/lib/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -20,19 +20,21 @@ * each VkDeviceMemory its own application mapping from the bo_mmap * primitive, dropped only by that object's unmap, reserve unmap and free. * - * This test drives the real production mapping code: agx_bo_map exactly as - * defined in agx_device.h with the bind expression of hk_image_plane_bind, - * the per-memory primitive expressions of hk_MapMemory2KHR, the per-memory - * teardown of hk_UnmapMemory2KHR and hk_FreeMemory, and the real - * agx_bo_unreference. The only mock is the kernel mmap: mock_bo_mmap backs - * the BO with one tmpfile (ftruncate to the BO size) and hands out - * MAP_SHARED mappings of it, so two live mappings of the same BO show the - * same bytes, like two CPU mappings of one GEM object. The placed target is - * a PROT_NONE reservation that stays mapped until the MAP_FIXED placed map - * replaces it. Every resource failure FAILs the case; there are no SKIP - * paths. There is no GPU, no device node and no Vulkan entrypoint; those - * are covered by the admitted device gate (dEQP-VK host-copy coverage with - * a placed map after image bind), not here. + * This test drives the real production mapping code shared with the driver: + * hk_memory_map_from_bo, hk_memory_unmap and hk_memory_unmap_reserve from + * hk_device_memory.h operating on real struct hk_device_memory objects, plus + * agx_bo_map exactly as defined in agx_device.h with the bind expression of + * hk_image_plane_bind, and the real agx_bo_unreference. A mutation of the + * shared helpers back to BO-owned application mappings therefore fails these + * cases. The only mock is the kernel mmap: mock_bo_mmap backs the BO with + * one tmpfile (ftruncate to the BO size) and hands out MAP_SHARED mappings + * of it, so two live mappings of the same BO show the same bytes, like two + * CPU mappings of one GEM object. The placed target is a PROT_NONE + * reservation that stays mapped until the MAP_FIXED placed map replaces it. + * Every resource failure FAILs the case; there are no SKIP paths. There is + * no GPU, no device node and no Vulkan entrypoint; those are covered by the + * admitted device gate (dEQP-VK host-copy coverage with a placed map after + * image bind), not here. * * The stable-map case pins the decoder contract: the internal mapping must * exist, match agx_bo_map, stay pointer-stable and coherent through the @@ -58,7 +60,7 @@ #include #include -#include "agx_device.h" +#include "hk_device_memory.h" /* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); * a placed map covers the allocation only. Same numbers as the PR: 0x100000 @@ -225,6 +227,28 @@ guarded_copy(void *dst, const void *src, size_t n) goto out; \ } while (0) +static void +mock_mem_init(struct hk_device_memory *mem, struct agx_bo *bo) +{ + memset(mem, 0, sizeof(*mem)); + mem->bo = bo; +} + +/* The memory object must own exactly the mapping the shared helper made. */ +#define ASSERT_MAPPED(mem, want_ptr, want_size) \ + do { \ + if ((mem)->map != (want_ptr) || (mem)->map_size != (want_size)) \ + CASE_FAIL("memory object does not own its mapping: map=%p " \ + "map_size=%zu", (mem)->map, (mem)->map_size); \ + } while (0) + +#define ASSERT_UNMAPPED(mem) \ + do { \ + if ((mem)->map != NULL || (mem)->map_size != 0) \ + CASE_FAIL("memory object mapping state not cleared: map=%p " \ + "map_size=%zu", (mem)->map, (mem)->map_size); \ + } while (0) + static int run_case(const char *name, unsigned flags) { @@ -234,14 +258,21 @@ run_case(const char *name, unsigned flags) if (!mock_bo_init(&m, BO_SIZE)) CASE_FAIL("tmpfile/ftruncate for the BO failed"); + /* Real per-memory objects: an earlier memory object mapping the BO + * plainly, and the one whose placed mapping this case tears down. */ + struct hk_device_memory pre_mem, mem; + mock_mem_init(&pre_mem, &m.bo); + mock_mem_init(&mem, &m.bo); + /* Optional plain application map before bind: a mapping owned by its own - * memory object (the hk_MapMemory2KHR ordinary expression: a fresh - * mapping from the bo_mmap primitive, never stored on the BO). */ + * memory object (the hk_MapMemory2KHR ordinary expression via the shared + * helper). */ uint8_t *app_map = NULL; if (flags & CASE_MAP_FIRST) { - app_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); + app_map = hk_memory_map_from_bo(&m.dev, &pre_mem, NULL, m.bo.size); if (app_map == NULL) CASE_FAIL("plain application map returned NULL"); + ASSERT_MAPPED(&pre_mem, app_map, m.bo.size); } /* Bind (hk_image.c:1333-1335): internal mapping retained by the image. */ @@ -267,16 +298,16 @@ run_case(const char *name, unsigned flags) CASE_FAIL("application and internal mappings disagree on BO contents"); /* Placed map at the reserved address: a mapping owned by its own memory - * object (the hk_MapMemory2KHR placed expression: the primitive at the - * requested fixed address). */ + * object (the hk_MapMemory2KHR placed expression via the shared helper). */ void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed, ALLOC_SIZE); + uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); } + ASSERT_MAPPED(&mem, placed, ALLOC_SIZE); /* Placed/internal coherence: the placed mapping covers the allocation and * must show the same BO bytes at the bound offsets. */ @@ -292,15 +323,12 @@ run_case(const char *name, unsigned flags) * (hk_UnmapMemory2KHR): reserve overlays exactly that mapping, a plain * unmap releases it. The stable BO mapping is never touched. */ if (flags & CASE_RESERVE) { - void *err = mmap(mem_map, ALLOC_SIZE, PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) + if (!hk_memory_unmap_reserve(&mem)) CASE_FAIL("reserve overlay mmap failed"); - mem_map = NULL; } else { - munmap(mem_map, ALLOC_SIZE); - mem_map = NULL; + hk_memory_unmap(&mem); } + ASSERT_UNMAPPED(&mem); /* Lifetime: host-copy read through the retained pointers (hk_image.c:1523). * The placed map and unmap must never touch the internal mapping. */ @@ -353,22 +381,27 @@ stable_map_survives_application_maps(void) memset(stable, 0x77, CANARY_LEN); /* Application placed map over the same BO: a mapping owned by its own - * memory object (the hk_MapMemory2KHR placed expression). */ + * memory object (the hk_MapMemory2KHR placed expression via the shared + * helper). */ + struct hk_device_memory mem; + mock_mem_init(&mem, &m.bo); + void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed, ALLOC_SIZE); + uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); } + ASSERT_MAPPED(&mem, placed, ALLOC_SIZE); if (memcmp(mem_map, stable, CANARY_LEN) != 0) CASE_FAIL("placed mapping disagrees with the stable BO mapping"); /* Application unmap of that memory object's mapping * (hk_UnmapMemory2KHR). */ - munmap(mem_map, ALLOC_SIZE); - mem_map = NULL; + hk_memory_unmap(&mem); + ASSERT_UNMAPPED(&mem); /* The stable mapping survives: same pointer, same bytes. */ if (m.bo._map != (void *)stable) @@ -407,29 +440,37 @@ shared_bo_importers_map_independently(void) CASE_FAIL("tmpfile/ftruncate for the BO failed"); m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ - /* Importer A: placed map (hk_MapMemory2KHR placed expression, fixed - * address). */ + struct hk_device_memory mem_a, mem_b; + mock_mem_init(&mem_a, &m.bo); + mock_mem_init(&mem_b, &m.bo); + + /* Importer A: placed map (hk_MapMemory2KHR placed expression via the + * shared helper, fixed address). */ void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed_a, ALLOC_SIZE); + uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", (void *)map_a, placed_a); } + ASSERT_MAPPED(&mem_a, placed_a, ALLOC_SIZE); uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; memset(pat_a, 0x5A, sizeof(pat_a)); memset(pat_b, 0xC3, sizeof(pat_b)); memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); - /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression, NULL - * fixed, whole BO). It must be its own mapping, not importer A's. */ - uint8_t *map_b = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the + * shared helper, NULL fixed, whole BO). It must be its own mapping, not + * importer A's. */ + uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); /* Coherence: both mappings show the same BO contents. */ if (memcmp(map_b + BIND_OFF_A, pat_a, CANARY_LEN) != 0) @@ -442,8 +483,8 @@ shared_bo_importers_map_independently(void) /* Importer A unmaps its own mapping (hk_UnmapMemory2KHR). Only A's * mapping may die. */ - munmap(map_a, ALLOC_SIZE); - map_a = NULL; + hk_memory_unmap(&mem_a); + ASSERT_UNMAPPED(&mem_a); uint8_t readback[CANARY_LEN]; if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) @@ -477,24 +518,31 @@ free_one_importer_survivor_survives(void) CASE_FAIL("tmpfile/ftruncate for the BO failed"); m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ - /* Importer A: placed map (hk_MapMemory2KHR placed expression, fixed - * address). */ + struct hk_device_memory mem_a, mem_b; + mock_mem_init(&mem_a, &m.bo); + mock_mem_init(&mem_b, &m.bo); + + /* Importer A: placed map (hk_MapMemory2KHR placed expression via the + * shared helper, fixed address). */ void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = m.dev.ops.bo_mmap(&m.dev, &m.bo, placed_a, ALLOC_SIZE); + uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", (void *)map_a, placed_a); } + ASSERT_MAPPED(&mem_a, placed_a, ALLOC_SIZE); - /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression, NULL - * fixed). */ - uint8_t *map_b = m.dev.ops.bo_mmap(&m.dev, &m.bo, NULL, m.bo.size); + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the + * shared helper, NULL fixed). */ + uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; memset(pat_a, 0x5A, sizeof(pat_a)); @@ -503,15 +551,16 @@ free_one_importer_survivor_survives(void) memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); /* vkFreeMemory(A): the mapping belongs to the freed memory object, so - * hk_FreeMemory drops exactly A's mapping, and the real refcount drop - * returns the BO alive at refcount 1 for B. */ - munmap(map_a, ALLOC_SIZE); - map_a = NULL; + * hk_FreeMemory drops exactly A's mapping via the shared helper, and the + * real refcount drop returns the BO alive at refcount 1 for B. */ + hk_memory_unmap(&mem_a); + ASSERT_UNMAPPED(&mem_a); agx_bo_unreference(&m.dev, &m.bo); if (p_atomic_read(&m.bo.refcnt) != 1) CASE_FAIL("freeing one importer did not leave the shared BO at " "refcount 1 (got %d)", p_atomic_read(&m.bo.refcnt)); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); /* Survivor B: mapping still valid, both canaries coherent. */ uint8_t readback[CANARY_LEN]; @@ -536,6 +585,49 @@ free_one_importer_survivor_survives(void) return rc; } +/* Boundary case: the shared helpers must treat a never-mapped memory object + * as a no-op (hk_UnmapMemory2KHR on unmapped memory must not touch the BO's + * stable mapping), and a later real map on the same object must work. */ +static int +unmap_without_map_is_noop(void) +{ + const char *name = "unmap_without_map_is_noop"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + struct hk_device_memory mem; + mock_mem_init(&mem, &m.bo); + ASSERT_UNMAPPED(&mem); + + hk_memory_unmap(&mem); + if (!hk_memory_unmap_reserve(&mem)) + CASE_FAIL("reserve unmap of a never-mapped object failed"); + ASSERT_UNMAPPED(&mem); + + /* The BO's stable mapping is untouched and a real map works after. */ + uint8_t *stable = agx_bo_map(&m.bo); + if (stable == NULL) + CASE_FAIL("internal map returned NULL"); + ASSERT_UNMAPPED(&mem); + + uint8_t *map = hk_memory_map_from_bo(&m.dev, &mem, NULL, m.bo.size); + if (map == NULL) + CASE_FAIL("map after no-op unmaps returned NULL"); + ASSERT_MAPPED(&mem, map, m.bo.size); + hk_memory_unmap(&mem); + ASSERT_UNMAPPED(&mem); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + int main(void) { @@ -544,6 +636,7 @@ main(void) failures += stable_map_survives_application_maps(); failures += shared_bo_importers_map_independently(); failures += free_one_importer_survivor_survives(); + failures += unmap_without_map_is_noop(); for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) failures += run_case(cases[i].name, cases[i].flags); From 4bd01db4499701a286adf0f7d0338817d2feb0cb Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:10:00 -0500 Subject: [PATCH 13/19] hk: keep app mappings independent of stable BO map Every VkDeviceMemory application map takes the backend-owned mapping path, ordinary as well as placed, even when a stable internal BO map already exists; image/decoder users keep that stable map untouched. Map, unmap, reserve and free operate only on the per-memory mapping. No mapping-count statistics exist in the backends, so app maps cannot skew them; command-execution stats are unaffected. Add a CPU regression asserting an ordinary app map leaves bo->_map empty before bind and the later internal map has a different address, alongside the placed-map and shared-importer lifetime cases. --- src/asahi/vulkan/hk_device_memory.c | 93 +--- src/asahi/vulkan/hk_device_memory.h | 44 +- src/asahi/vulkan/meson.build | 14 + src/asahi/vulkan/tests/test-placed-map.c | 663 +++++++++++++++++++++++ 4 files changed, 730 insertions(+), 84 deletions(-) create mode 100644 src/asahi/vulkan/tests/test-placed-map.c diff --git a/src/asahi/vulkan/hk_device_memory.c b/src/asahi/vulkan/hk_device_memory.c index 0de931119c28..f0189b988c66 100644 --- a/src/asahi/vulkan/hk_device_memory.c +++ b/src/asahi/vulkan/hk_device_memory.c @@ -18,7 +18,6 @@ #include "util/os_misc.h" #include -#include /* GPU memory is system memory that the kernel cannot evict or swap, and * heapBudget is only advisory, so the heap limit alone lets a process (or @@ -376,15 +375,7 @@ hk_FreeMemory(VkDevice device, VkDeviceMemory _mem, if (mem->bo->flags & (AGX_BO_SHAREABLE | AGX_BO_SHARED)) hk_remove_ext_bo(dev, mem->bo); - /* Freeing a mapped memory object unmaps it. The shared mapping goes with - * the BO; an owned one belongs to this object alone. */ - if (mem->map_owned) - agx_bo_unmap_owned(mem->bo, mem->map, mem->map_size); - else if (mem->map && mem->map_placed) { - dev->dev.ops.bo_munmap(&dev->dev, mem->bo); - mem->bo->_map = NULL; - mem->bo->_map_size = 0; - } + hk_memory_unmap(mem); agx_bo_unreference(&dev->dev, mem->bo); @@ -446,8 +437,7 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, "A placed mapping must cover exactly the allocation."); } - /* A backend unable to create independent mappings must not relocate a - * mapping that another imported memory object may still be using. */ + /* Preserve the backend alias guard before asking for an independent map. */ if (fixed_addr && !dev->dev.ops.bo_mmap_independent && mem->bo->_map && p_atomic_read(&mem->bo->refcnt) > 1) { *ppData = NULL; @@ -455,24 +445,11 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, "Cannot relocate a shared mapping on this backend."); } - /* Bound a placed mapping by the memory object, not the BO: the BO cache - * may have handed us one up to twice as large, and the application has - * reserved only mem->vk.size at this address. - * - * An exported allocation and its imports into this device are distinct - * memory objects over one agx_bo. Each maps, unmaps, frees and reserves - * independently, so each gets a mapping of its own rather than the BO's - * shared one, which unmapping any of them would otherwise tear down - * under the others. A BO only one memory object can reach keeps the - * cached mapping. */ - if (mem->bo->flags & (AGX_BO_SHARED | AGX_BO_SHAREABLE)) { - mem->map = agx_bo_map_owned(mem->bo, fixed_addr, mem->vk.size, - &mem->map_size); - mem->map_owned = mem->map != NULL; - } - if (!mem->map_owned) - mem->map = agx_bo_map_placed(mem->bo, fixed_addr, mem->vk.size); - if (mem->map == NULL) { + /* Application maps must never replace the stable mapping retained by + * image host-copy or decoder users, even when that mapping is absent yet. + */ + size_t map_size = fixed_addr ? mem->vk.size : mem->bo->size; + if (hk_memory_map_from_bo(mem, fixed_addr, map_size) == NULL) { *ppData = NULL; return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "failed to map memory BO handle %u", @@ -480,21 +457,12 @@ hk_MapMemory2KHR(VkDevice device, const VkMemoryMapInfoKHR *pMemoryMapInfo, } if (fixed_addr && mem->map != fixed_addr) { - if (mem->map_owned) - agx_bo_unmap_owned(mem->bo, mem->map, mem->map_size); - else { - dev->dev.ops.bo_munmap(&dev->dev, mem->bo); - mem->bo->_map = NULL; - mem->bo->_map_size = 0; - } - mem->map = NULL; - mem->map_owned = false; - mem->map_size = 0; + hk_memory_unmap(mem); *ppData = NULL; return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Placed mapping returned an unexpected address."); } - mem->map_placed = fixed_addr != NULL; + *ppData = mem->map + offset; return VK_SUCCESS; @@ -509,54 +477,15 @@ hk_UnmapMemory2KHR(VkDevice device, if (mem == NULL || mem->map == NULL) return VK_SUCCESS; - if (mem->map_owned) { - if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { - VK_FROM_HANDLE(hk_device, dev, device); - - /* Exactly this object's mapping, as below. */ - void *err = mmap(mem->map, mem->map_size, PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) { - return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to map over original mapping"); - } - } else { - agx_bo_unmap_owned(mem->bo, mem->map, mem->map_size); - } - mem->map = NULL; - mem->map_size = 0; - mem->map_owned = false; - mem->map_placed = false; - return VK_SUCCESS; - } - if (pMemoryUnmapInfo->flags & VK_MEMORY_UNMAP_RESERVE_BIT_EXT) { VK_FROM_HANDLE(hk_device, dev, device); - - /* Reserve exactly what was mapped. A placed map covers the - * VkDeviceMemory's allocationSize, not the cached BO's size, and - * PROT_NONE'ing the difference lands on address space the application - * reserved for itself -- the guard pages dEQP-VK.memory.map_placed.* - * fills and reads back. */ - void *err = mmap(mem->bo->_map, agx_bo_map_size(mem->bo), PROT_NONE, - MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); - if (err == MAP_FAILED) { + if (!hk_memory_unmap_reserve(mem)) return vk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, "Failed to map over original mapping"); - } - mem->bo->_map = NULL; - mem->bo->_map_size = 0; } else { - if (mem->bo->_map) { - munmap(mem->bo->_map, agx_bo_map_size(mem->bo)); - mem->bo->_map = NULL; - mem->bo->_map_size = 0; - } + hk_memory_unmap(mem); } - mem->map = NULL; - mem->map_placed = false; - return VK_SUCCESS; } diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index 64b6e7da8261..dc5ef8c0e801 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -9,10 +9,14 @@ #include "hk_private.h" +#include "asahi/lib/agx_device.h" + #include "vk_device_memory.h" #include "util/list.h" +#include + struct hk_device; struct hk_image_plane; @@ -31,10 +35,46 @@ struct hk_device_memory { /* map is this object's own mapping of map_size bytes rather than the * BO's shared one: the BO may be aliased by other memory objects. */ size_t map_size; - bool map_owned; - bool map_placed; }; + +/* Application mappings belong to one VkDeviceMemory, never the shared BO. */ +static inline void * +hk_memory_map_from_bo(struct agx_device *dev, struct hk_device_memory *mem, + void *fixed_addr, size_t length) +{ + (void)dev; + mem->map = agx_bo_map_owned(mem->bo, fixed_addr, length, &mem->map_size); + return mem->map; +} + +static inline void +hk_memory_unmap(struct hk_device_memory *mem) +{ + if (mem->map) { + agx_bo_unmap_owned(mem->bo, mem->map, mem->map_size); + mem->map = NULL; + mem->map_size = 0; + } +} + +static inline bool +hk_memory_unmap_reserve(struct hk_device_memory *mem) +{ + if (!mem->map) + return true; + + void *err = mmap(mem->map, mem->map_size, PROT_NONE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + if (err == MAP_FAILED) + return false; + + mem->map = NULL; + mem->map_size = 0; + return true; +} + + VK_DEFINE_NONDISP_HANDLE_CASTS(hk_device_memory, vk.base, VkDeviceMemory, VK_OBJECT_TYPE_DEVICE_MEMORY) diff --git a/src/asahi/vulkan/meson.build b/src/asahi/vulkan/meson.build index 7519b6e2716f..0e4df071658f 100644 --- a/src/asahi/vulkan/meson.build +++ b/src/asahi/vulkan/meson.build @@ -127,6 +127,20 @@ hk_drirc = custom_target( ) if with_tests + test('hk_placed_map_lifetime', + executable('hk-placed-map-lifetime', + files('tests/test-placed-map.c'), + c_args : [c_msvc_compat_args, no_override_init_args, + '-Wno-c2x-extensions'], + gnu_symbol_visibility : 'hidden', + include_directories : [ + inc_gallium, inc_gallium_aux, inc_include, inc_src, + inc_asahi, inc_virtio_gpu, inc_virtio_vdrm, + ], + dependencies : [hk_deps, idep_mesautil], + link_with : [libasahi_lib]), + suite : ['asahi']) + test('hk_execution_allowance', executable('hk_execution_allowance_test', files('tests/test-hk-execution-allowance.c'), diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c new file mode 100644 index 000000000000..53b2cc32f43c --- /dev/null +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -0,0 +1,663 @@ +/* + * Copyright 2026 Joshua Warren + * SPDX-License-Identifier: MIT + */ + +/* + * CPU-only regression for the omacom/mesa#8 review finding + * (https://github.com/omacom/mesa/pull/8#discussion_r4418383484) and its + * follow-ups: + * + * A non-sparse image bind retains an internal host-copy mapping + * (hk_image_plane_bind, hk_image.c:1334: plane->map = agx_bo_map(mem->bo) + + * offset) without setting mem->map or taking a BO reference. While the + * application mapping lived on the BO, any vkMapMemory2KHR placed map, + * vkUnmapMemory2KHR (plain or VK_MEMORY_UNMAP_RESERVE_BIT_EXT) or + * vkFreeMemory of the same memory destroyed that retained pointer, and two + * memory objects sharing an imported BO shared one replaceable mapping. The + * fix gives the BO a single stable internal mapping (agx_bo_map -> + * bo->_map, read directly by decode.c and populated by wrap.c) and gives + * each VkDeviceMemory its own application mapping from the bo_mmap + * primitive, dropped only by that object's unmap, reserve unmap and free. + * + * This test drives the real production mapping code shared with the driver: + * hk_memory_map_from_bo, hk_memory_unmap and hk_memory_unmap_reserve from + * hk_device_memory.h operating on real struct hk_device_memory objects, plus + * agx_bo_map exactly as defined in agx_device.h with the bind expression of + * hk_image_plane_bind, and the real agx_bo_unreference. A mutation of the + * shared helpers back to BO-owned application mappings therefore fails these + * cases. The only mock is the kernel mmap: mock_bo_mmap backs the BO with + * one tmpfile (ftruncate to the BO size) and hands out MAP_SHARED mappings + * of it, so two live mappings of the same BO show the same bytes, like two + * CPU mappings of one GEM object. The placed target is a PROT_NONE + * reservation that stays mapped until the MAP_FIXED placed map replaces it. + * Every resource failure FAILs the case; there are no SKIP paths. There is + * no GPU, no device node and no Vulkan entrypoint; those are covered by the + * admitted device gate (dEQP-VK host-copy coverage with a placed map after + * image bind), not here. + * + * The stable-map case pins the decoder contract: the internal mapping must + * exist, match agx_bo_map, stay pointer-stable and coherent through the + * application map lifecycle. The two-importer and free cases pin shared-BO + * lifetime: distinct mappings per memory object, mutual content coherence, + * and a survivor whose mapping outlives the other importer's unmap or + * free. The heap accounting, ext-BO tracking and vk_device_memory_destroy + * tail of hk_FreeMemory need a live vk_device and stay behind the admitted + * device gate. + * + * Red was parent-observed on the pre-fix lineage (fixture heads 8af285bd30c + * and e14884a59ca: stable _map absent, importer B reusing importer A's + * placed mapping; the free case stopped at the same pointer-independence + * assertion before reaching the survivor checks). Passes with per-memory + * ownership. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "hk_device_memory.h" + +/* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); + * a placed map covers the allocation only. Same numbers as the PR: 0x100000 + * allocation in a 0x104000 BO. */ +#define ALLOC_SIZE 0x100000 +#define BO_SIZE 0x104000 + +/* Two images (or planes) bound to one memory object at different offsets, as + * hk_BindImageMemory2 allows. Both offsets sit inside ALLOC_SIZE so the + * placed mapping can be checked for the same contents. */ +#define BIND_OFF_A 0x1000 +#define BIND_OFF_B 0x80000 + +#define CANARY_LEN 0x1000 + +/* Case selection bits. Plain unmap (hk_device_memory.c:405) is the default + * application teardown; CASE_RESERVE uses the :394 overlay instead. */ +#define CASE_RESERVE (1u << 0) +#define CASE_ALIAS (1u << 1) +#define CASE_MAP_FIRST (1u << 2) + +static const struct { + const char *name; + unsigned flags; +} cases[] = { + {"placed_map_after_bind_then_unmap", 0}, + {"reserve_unmap_after_bind", CASE_RESERVE}, + {"aliased_images_survive_placed_map", CASE_ALIAS}, + /* The app-first order must leave the internal BO map independent. */ + {"map_before_bind_keeps_maps_independent", CASE_MAP_FIRST}, +}; + +#define MOCK_MAX_MAPS 8 + +struct mock_map { + void *addr; + size_t len; +}; + +struct mock_state { + /* Must stay first: ops callbacks only get the agx_device pointer. */ + struct agx_device dev; + struct agx_bo bo; + FILE *file; + struct mock_map maps[MOCK_MAX_MAPS]; + unsigned map_count; +}; + +static void +mock_track(struct mock_state *m, void *addr, size_t len) +{ + for (unsigned i = 0; i < m->map_count; i++) { + if (m->maps[i].addr == addr) { + m->maps[i].len = len; + return; + } + } + if (m->map_count == MOCK_MAX_MAPS) { + fprintf(stderr, "mock: mapping table full\n"); + exit(99); + } + m->maps[m->map_count].addr = addr; + m->maps[m->map_count].len = len; + m->map_count++; +} + +/* The single mock: the kernel side of a GEM mmap. One tmpfile is the BO + * storage; every mapping is MAP_SHARED over it, at fixed_addr with MAP_FIXED + * when requested. Body mirrors agx_bo_mmap (agx_device.c) minus the ioctls; + * like the production primitive it writes the mapping slot on the supplied BO + * copy, which exercises agx_bo_map_owned's real backend contract. */ +static void +mock_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, + size_t size) +{ + struct mock_state *m = (struct mock_state *)dev; + size = size ?: bo->size; + void *p = mmap(fixed_addr, size, PROT_READ | PROT_WRITE, + MAP_SHARED | (fixed_addr ? MAP_FIXED : 0), fileno(m->file), + 0); + bo->_map = p == MAP_FAILED ? NULL : p; + bo->_map_size = bo->_map ? size : 0; + if (bo->_map) + mock_track(m, bo->_map, size); +} + +static void +mock_bo_munmap(struct agx_device *dev, struct agx_bo *bo) +{ + (void)dev; + if (bo->_map) + munmap(bo->_map, agx_bo_map_size(bo)); +} + +static bool +mock_bo_init(struct mock_state *m, size_t bo_size) +{ + memset(m, 0, sizeof(*m)); + m->dev.ops.bo_munmap = mock_bo_munmap; + m->dev.ops.bo_mmap_independent = true; + m->dev.ops.bo_mmap = mock_bo_mmap; + m->bo.dev = &m->dev; + m->bo.size = bo_size; + m->bo.refcnt = 1; + + m->file = tmpfile(); + if (!m->file) + return false; + if (ftruncate(fileno(m->file), (off_t)bo_size) != 0) { + fclose(m->file); + m->file = NULL; + return false; + } + return true; +} + +/* Per-case cleanup: drop every mapping the shim made and the backing file. */ +static void +mock_bo_cleanup(struct mock_state *m) +{ + for (unsigned i = 0; i < m->map_count; i++) + munmap(m->maps[i].addr, m->maps[i].len); + if (m->file) + fclose(m->file); +} + +/* Reserve the placed target with PROT_NONE and keep it mapped: the MAP_FIXED + * placed map replaces it in place, never releasing the address first. */ +static void * +reserve_placed_address(struct mock_state *m, size_t size) +{ + void *p = mmap(NULL, size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (p == MAP_FAILED) + return NULL; + mock_track(m, p, size); + return p; +} + +static sigjmp_buf fault_env; +static volatile sig_atomic_t faulted; + +static void +fault_handler(int sig) +{ + (void)sig; + faulted = 1; + siglongjmp(fault_env, 1); +} + +/* Reads through a possibly-dangling retained pointer the way the host-copy + * paths do (hk_image.c:1523). Returns false when the access faults. */ +static bool +guarded_copy(void *dst, const void *src, size_t n) +{ + void *prev_segv = signal(SIGSEGV, fault_handler); + void *prev_bus = signal(SIGBUS, fault_handler); + + faulted = 0; + if (sigsetjmp(fault_env, 1) == 0) + memcpy(dst, src, n); + + signal(SIGSEGV, prev_segv); + signal(SIGBUS, prev_bus); + return !faulted; +} + +#define CASE_FAIL(...) \ + do { \ + printf("FAIL %s: ", name); \ + printf(__VA_ARGS__); \ + putchar('\n'); \ + goto out; \ + } while (0) + +static void +mock_mem_init(struct hk_device_memory *mem, struct agx_bo *bo) +{ + memset(mem, 0, sizeof(*mem)); + mem->bo = bo; +} + +/* The memory object must own exactly the mapping the shared helper made. */ +#define ASSERT_MAPPED(mem, want_ptr, want_size) \ + do { \ + if ((mem)->map != (want_ptr) || (mem)->map_size != (want_size)) \ + CASE_FAIL("memory object does not own its mapping: map=%p " \ + "map_size=%zu", (mem)->map, (mem)->map_size); \ + } while (0) + +#define ASSERT_UNMAPPED(mem) \ + do { \ + if ((mem)->map != NULL || (mem)->map_size != 0) \ + CASE_FAIL("memory object mapping state not cleared: map=%p " \ + "map_size=%zu", (mem)->map, (mem)->map_size); \ + } while (0) + +static int +run_case(const char *name, unsigned flags) +{ + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + /* Real per-memory objects: an earlier memory object mapping the BO + * plainly, and the one whose placed mapping this case tears down. */ + struct hk_device_memory pre_mem, mem; + mock_mem_init(&pre_mem, &m.bo); + mock_mem_init(&mem, &m.bo); + + /* Optional plain application map before bind: a mapping owned by its own + * memory object (the hk_MapMemory2KHR ordinary expression via the shared + * helper). */ + uint8_t *app_map = NULL; + if (flags & CASE_MAP_FIRST) { + app_map = hk_memory_map_from_bo(&m.dev, &pre_mem, NULL, m.bo.size); + if (app_map == NULL) + CASE_FAIL("plain application map returned NULL"); + ASSERT_MAPPED(&pre_mem, app_map, m.bo.size); + if (m.bo._map != NULL) + CASE_FAIL("application map populated stable BO mapping before bind"); + } + + /* Bind (hk_image.c:1333-1335): internal mapping retained by the image. */ + void *internal = agx_bo_map(&m.bo); + if (internal == NULL) + CASE_FAIL("internal map returned NULL"); + if ((flags & CASE_MAP_FIRST) && m.bo._map == app_map) + CASE_FAIL("internal map reused the earlier application mapping"); + uint8_t *image_a = (uint8_t *)internal + BIND_OFF_A; + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(image_a, pat_a, sizeof(pat_a)); + + uint8_t *image_b = NULL; + if (flags & CASE_ALIAS) { + image_b = (uint8_t *)agx_bo_map(&m.bo) + BIND_OFF_B; + memcpy(image_b, pat_b, sizeof(pat_b)); + } + + /* Coherence: the retained mapping and an earlier application mapping of + * the same BO must show the same bytes. */ + if (app_map != NULL && + memcmp(app_map + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("application and internal mappings disagree on BO contents"); + + /* Placed map at the reserved address: a mapping owned by its own memory + * object (the hk_MapMemory2KHR placed expression via the shared helper). */ + void *placed = reserve_placed_address(&m, ALLOC_SIZE); + if (placed == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); + if (mem_map != placed) { + CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, + placed); + } + ASSERT_MAPPED(&mem, placed, ALLOC_SIZE); + + /* Placed/internal coherence: the placed mapping covers the allocation and + * must show the same BO bytes at the bound offsets. */ + if (memcmp(mem_map + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("placed mapping does not show the BO contents at the bind " + "offset"); + if ((flags & CASE_ALIAS) && + memcmp(mem_map + BIND_OFF_B, pat_b, CANARY_LEN) != 0) + CASE_FAIL("placed mapping does not show the BO contents at the alias " + "offset"); + + /* Application teardown, owned by the mapped memory object + * (hk_UnmapMemory2KHR): reserve overlays exactly that mapping, a plain + * unmap releases it. The stable BO mapping is never touched. */ + if (flags & CASE_RESERVE) { + if (!hk_memory_unmap_reserve(&mem)) + CASE_FAIL("reserve overlay mmap failed"); + } else { + hk_memory_unmap(&mem); + } + ASSERT_UNMAPPED(&mem); + + /* Lifetime: host-copy read through the retained pointers (hk_image.c:1523). + * The placed map and unmap must never touch the internal mapping. */ + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, image_a, CANARY_LEN)) + CASE_FAIL("host copy through the retained image mapping faults after " + "placed map + unmap (omacom/mesa#8 r4418383484)"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("retained image mapping was destroyed or reused: canary " + "mismatch"); + if (flags & CASE_ALIAS) { + if (!guarded_copy(readback, image_b, CANARY_LEN)) + CASE_FAIL("aliased image's retained mapping faults after placed map " + "+ unmap"); + if (memcmp(readback, pat_b, CANARY_LEN) != 0) + CASE_FAIL("aliased image's retained mapping canary mismatch"); + } + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +/* Follow-up finding: decode.c (agxdecode_grab_mapped :102, fetch :136, + * cmdstream :976) and wrap.c (:210, :230) read and populate bo->_map directly. + * The stable BO mapping must exist, match agx_bo_map, stay pointer-stable and + * coherent through the application map lifecycle. */ +static int +stable_map_survives_application_maps(void) +{ + const char *name = "stable_map_survives_application_maps"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + /* Decoder-style stable mapping. */ + uint8_t *stable = agx_bo_map(&m.bo); + if (stable == NULL) + CASE_FAIL("internal map returned NULL"); + if (m.bo._map == NULL) + CASE_FAIL("internal map did not populate the stable BO mapping _map " + "read by decode.c and wrap.c"); + if (m.bo._map != (void *)stable) + CASE_FAIL("agx_bo_map result does not match the stable BO mapping"); + memset(stable, 0x77, CANARY_LEN); + + /* Application placed map over the same BO: a mapping owned by its own + * memory object (the hk_MapMemory2KHR placed expression via the shared + * helper). */ + struct hk_device_memory mem; + mock_mem_init(&mem, &m.bo); + + void *placed = reserve_placed_address(&m, ALLOC_SIZE); + if (placed == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); + if (mem_map != placed) { + CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, + placed); + } + ASSERT_MAPPED(&mem, placed, ALLOC_SIZE); + if (memcmp(mem_map, stable, CANARY_LEN) != 0) + CASE_FAIL("placed mapping disagrees with the stable BO mapping"); + + /* Application unmap of that memory object's mapping + * (hk_UnmapMemory2KHR). */ + hk_memory_unmap(&mem); + ASSERT_UNMAPPED(&mem); + + /* The stable mapping survives: same pointer, same bytes. */ + if (m.bo._map != (void *)stable) + CASE_FAIL("application maps replaced the stable BO mapping pointer"); + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, stable, CANARY_LEN)) + CASE_FAIL("stable BO mapping faults after application placed map + " + "unmap"); + for (size_t i = 0; i < CANARY_LEN; i++) { + if (readback[i] != 0x77) + CASE_FAIL("stable BO mapping was destroyed or reused: canary " + "mismatch at offset %zu", + i); + } + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +/* Follow-up finding: two VkDeviceMemory importers share one BO. An ordinary + * map of the second importer must not reuse the first importer's placed + * mapping, and the first importer's unmap must not destroy the second's + * mapping. Both mappings must show the same BO contents. */ +static int +shared_bo_importers_map_independently(void) +{ + const char *name = "shared_bo_importers_map_independently"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ + + struct hk_device_memory mem_a, mem_b; + mock_mem_init(&mem_a, &m.bo); + mock_mem_init(&mem_b, &m.bo); + + /* Importer A: placed map (hk_MapMemory2KHR placed expression via the + * shared helper, fixed address). */ + void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); + if (placed_a == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + ALLOC_SIZE); + if (map_a != placed_a) { + CASE_FAIL("importer A placed map returned %p, requested %p", + (void *)map_a, placed_a); + } + ASSERT_MAPPED(&mem_a, placed_a, ALLOC_SIZE); + + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); + + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the + * shared helper, NULL fixed, whole BO). It must be its own mapping, not + * importer A's. */ + uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); + if (map_b == NULL) + CASE_FAIL("importer B map returned NULL"); + if (map_b == map_a) + CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); + + /* Coherence: both mappings show the same BO contents. */ + if (memcmp(map_b + BIND_OFF_A, pat_a, CANARY_LEN) != 0) + CASE_FAIL("importer B mapping disagrees with importer A on BO " + "contents"); + memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); + if (memcmp(map_a + BIND_OFF_B, pat_b, CANARY_LEN) != 0) + CASE_FAIL("importer A mapping disagrees with importer B on BO " + "contents"); + + /* Importer A unmaps its own mapping (hk_UnmapMemory2KHR). Only A's + * mapping may die. */ + hk_memory_unmap(&mem_a); + ASSERT_UNMAPPED(&mem_a); + + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) + CASE_FAIL("importer B mapping faults after importer A unmap"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("importer B mapping was destroyed or reused by importer A's " + "unmap: canary mismatch"); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +/* The user-required free path: two memory objects share one imported BO; + * freeing one runs the BO-relevant hk_FreeMemory sequence (placed-unmap + * condition, hk_device_memory.c:283, plus the real agx_bo_unreference) and + * the survivor's mapping must stay valid and coherent. The heap accounting, + * ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a + * live vk_device and stay behind the admitted device gate. */ +static int +free_one_importer_survivor_survives(void) +{ + const char *name = "free_one_importer_survivor_survives"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + m.bo.refcnt = 2; /* imported share: two memory objects, one BO */ + + struct hk_device_memory mem_a, mem_b; + mock_mem_init(&mem_a, &m.bo); + mock_mem_init(&mem_b, &m.bo); + + /* Importer A: placed map (hk_MapMemory2KHR placed expression via the + * shared helper, fixed address). */ + void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); + if (placed_a == NULL) + CASE_FAIL("could not reserve the placed target address"); + uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + ALLOC_SIZE); + if (map_a != placed_a) { + CASE_FAIL("importer A placed map returned %p, requested %p", + (void *)map_a, placed_a); + } + ASSERT_MAPPED(&mem_a, placed_a, ALLOC_SIZE); + + /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the + * shared helper, NULL fixed). */ + uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); + if (map_b == NULL) + CASE_FAIL("importer B map returned NULL"); + if (map_b == map_a) + CASE_FAIL("importer B ordinary map reused importer A's placed mapping"); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); + + uint8_t pat_a[CANARY_LEN], pat_b[CANARY_LEN]; + memset(pat_a, 0x5A, sizeof(pat_a)); + memset(pat_b, 0xC3, sizeof(pat_b)); + memcpy(map_a + BIND_OFF_A, pat_a, sizeof(pat_a)); + memcpy(map_b + BIND_OFF_B, pat_b, sizeof(pat_b)); + + /* vkFreeMemory(A): the mapping belongs to the freed memory object, so + * hk_FreeMemory drops exactly A's mapping via the shared helper, and the + * real refcount drop returns the BO alive at refcount 1 for B. */ + hk_memory_unmap(&mem_a); + ASSERT_UNMAPPED(&mem_a); + agx_bo_unreference(&m.dev, &m.bo); + if (p_atomic_read(&m.bo.refcnt) != 1) + CASE_FAIL("freeing one importer did not leave the shared BO at " + "refcount 1 (got %d)", + p_atomic_read(&m.bo.refcnt)); + ASSERT_MAPPED(&mem_b, map_b, m.bo.size); + + /* Survivor B: mapping still valid, both canaries coherent. */ + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, map_b + BIND_OFF_A, CANARY_LEN)) + CASE_FAIL("survivor mapping faults after the other importer was " + "freed"); + if (memcmp(readback, pat_a, CANARY_LEN) != 0) + CASE_FAIL("survivor mapping was destroyed or reused by the other " + "importer's free: canary mismatch"); + if (!guarded_copy(readback, map_b + BIND_OFF_B, CANARY_LEN)) + CASE_FAIL("survivor mapping faults on its own written range after " + "the other importer was freed"); + if (memcmp(readback, pat_b, CANARY_LEN) != 0) + CASE_FAIL("survivor's own written range corrupted after the other " + "importer was freed"); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +/* Boundary case: the shared helpers must treat a never-mapped memory object + * as a no-op (hk_UnmapMemory2KHR on unmapped memory must not touch the BO's + * stable mapping), and a later real map on the same object must work. */ +static int +unmap_without_map_is_noop(void) +{ + const char *name = "unmap_without_map_is_noop"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + struct hk_device_memory mem; + mock_mem_init(&mem, &m.bo); + ASSERT_UNMAPPED(&mem); + + hk_memory_unmap(&mem); + if (!hk_memory_unmap_reserve(&mem)) + CASE_FAIL("reserve unmap of a never-mapped object failed"); + ASSERT_UNMAPPED(&mem); + + /* The BO's stable mapping is untouched and a real map works after. */ + uint8_t *stable = agx_bo_map(&m.bo); + if (stable == NULL) + CASE_FAIL("internal map returned NULL"); + ASSERT_UNMAPPED(&mem); + + uint8_t *map = hk_memory_map_from_bo(&m.dev, &mem, NULL, m.bo.size); + if (map == NULL) + CASE_FAIL("map after no-op unmaps returned NULL"); + ASSERT_MAPPED(&mem, map, m.bo.size); + hk_memory_unmap(&mem); + ASSERT_UNMAPPED(&mem); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + +int +main(void) +{ + int failures = 0; + + failures += stable_map_survives_application_maps(); + failures += shared_bo_importers_map_independently(); + failures += free_one_importer_survivor_survives(); + failures += unmap_without_map_is_noop(); + + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) + failures += run_case(cases[i].name, cases[i].flags); + + if (failures) { + printf("%d case(s) failed: an application placed map or unmap destroys " + "the internal mapping retained by a bound image\n", + failures); + return 1; + } + return 0; +} From 12781ec481d827b3f27724c0e5d8e7a839b735ca Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:13:29 -0500 Subject: [PATCH 14/19] hk: drop the unused device parameter from the memory-map helper hk_MapMemory2KHR's three-argument call is the contract; the regression called the same helper with a stray device pointer. Remove the parameter and its (void) silencer from the header and update all eight test callsites, so the header, driver and test agree on one signature. --- src/asahi/vulkan/hk_device_memory.h | 5 ++--- src/asahi/vulkan/tests/test-placed-map.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/asahi/vulkan/hk_device_memory.h b/src/asahi/vulkan/hk_device_memory.h index dc5ef8c0e801..600186f15135 100644 --- a/src/asahi/vulkan/hk_device_memory.h +++ b/src/asahi/vulkan/hk_device_memory.h @@ -40,10 +40,9 @@ struct hk_device_memory { /* Application mappings belong to one VkDeviceMemory, never the shared BO. */ static inline void * -hk_memory_map_from_bo(struct agx_device *dev, struct hk_device_memory *mem, - void *fixed_addr, size_t length) +hk_memory_map_from_bo(struct hk_device_memory *mem, void *fixed_addr, + size_t length) { - (void)dev; mem->map = agx_bo_map_owned(mem->bo, fixed_addr, length, &mem->map_size); return mem->map; } diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index 53b2cc32f43c..1e4ff1711363 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -277,7 +277,7 @@ run_case(const char *name, unsigned flags) * helper). */ uint8_t *app_map = NULL; if (flags & CASE_MAP_FIRST) { - app_map = hk_memory_map_from_bo(&m.dev, &pre_mem, NULL, m.bo.size); + app_map = hk_memory_map_from_bo(&pre_mem, NULL, m.bo.size); if (app_map == NULL) CASE_FAIL("plain application map returned NULL"); ASSERT_MAPPED(&pre_mem, app_map, m.bo.size); @@ -314,7 +314,7 @@ run_case(const char *name, unsigned flags) void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); + uint8_t *mem_map = hk_memory_map_from_bo(&mem, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); @@ -401,7 +401,7 @@ stable_map_survives_application_maps(void) void *placed = reserve_placed_address(&m, ALLOC_SIZE); if (placed == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *mem_map = hk_memory_map_from_bo(&m.dev, &mem, placed, ALLOC_SIZE); + uint8_t *mem_map = hk_memory_map_from_bo(&mem, placed, ALLOC_SIZE); if (mem_map != placed) { CASE_FAIL("placed map returned %p, requested %p", (void *)mem_map, placed); @@ -461,7 +461,7 @@ shared_bo_importers_map_independently(void) void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + uint8_t *map_a = hk_memory_map_from_bo(&mem_a, placed_a, ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", @@ -477,7 +477,7 @@ shared_bo_importers_map_independently(void) /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the * shared helper, NULL fixed, whole BO). It must be its own mapping, not * importer A's. */ - uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); + uint8_t *map_b = hk_memory_map_from_bo(&mem_b, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) @@ -539,7 +539,7 @@ free_one_importer_survivor_survives(void) void *placed_a = reserve_placed_address(&m, ALLOC_SIZE); if (placed_a == NULL) CASE_FAIL("could not reserve the placed target address"); - uint8_t *map_a = hk_memory_map_from_bo(&m.dev, &mem_a, placed_a, + uint8_t *map_a = hk_memory_map_from_bo(&mem_a, placed_a, ALLOC_SIZE); if (map_a != placed_a) { CASE_FAIL("importer A placed map returned %p, requested %p", @@ -549,7 +549,7 @@ free_one_importer_survivor_survives(void) /* Importer B: ordinary map (hk_MapMemory2KHR ordinary expression via the * shared helper, NULL fixed). */ - uint8_t *map_b = hk_memory_map_from_bo(&m.dev, &mem_b, NULL, m.bo.size); + uint8_t *map_b = hk_memory_map_from_bo(&mem_b, NULL, m.bo.size); if (map_b == NULL) CASE_FAIL("importer B map returned NULL"); if (map_b == map_a) @@ -625,7 +625,7 @@ unmap_without_map_is_noop(void) CASE_FAIL("internal map returned NULL"); ASSERT_UNMAPPED(&mem); - uint8_t *map = hk_memory_map_from_bo(&m.dev, &mem, NULL, m.bo.size); + uint8_t *map = hk_memory_map_from_bo(&mem, NULL, m.bo.size); if (map == NULL) CASE_FAIL("map after no-op unmaps returned NULL"); ASSERT_MAPPED(&mem, map, m.bo.size); From 2f8ebd0dd44e60fc7735f5a707167aea701dd0ac Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:45:06 -0500 Subject: [PATCH 15/19] hk: register placed-map test after its dependencies Declare the regression after hk_deps exists and link the Asahi layout and compiler libraries required by libasahi_lib. Guard the stable-map coherence read so the pre-fix control reports the dangling retained map as a lifetime failure instead of crashing in memcmp. --- src/asahi/vulkan/meson.build | 29 ++++++++++++------------ src/asahi/vulkan/tests/test-placed-map.c | 6 +++-- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/asahi/vulkan/meson.build b/src/asahi/vulkan/meson.build index 0e4df071658f..4c9df628e1df 100644 --- a/src/asahi/vulkan/meson.build +++ b/src/asahi/vulkan/meson.build @@ -127,20 +127,6 @@ hk_drirc = custom_target( ) if with_tests - test('hk_placed_map_lifetime', - executable('hk-placed-map-lifetime', - files('tests/test-placed-map.c'), - c_args : [c_msvc_compat_args, no_override_init_args, - '-Wno-c2x-extensions'], - gnu_symbol_visibility : 'hidden', - include_directories : [ - inc_gallium, inc_gallium_aux, inc_include, inc_src, - inc_asahi, inc_virtio_gpu, inc_virtio_vdrm, - ], - dependencies : [hk_deps, idep_mesautil], - link_with : [libasahi_lib]), - suite : ['asahi']) - test('hk_execution_allowance', executable('hk_execution_allowance_test', files('tests/test-hk-execution-allowance.c'), @@ -468,6 +454,21 @@ hk_deps = [ idep_libagx, idep_libagx_shaders_h, ] +if with_tests + test('hk_placed_map_lifetime', + executable('hk-placed-map-lifetime', + files('tests/test-placed-map.c'), + c_args : [c_msvc_compat_args, no_override_init_args, + '-Wno-c2x-extensions'], + gnu_symbol_visibility : 'hidden', + include_directories : [ + inc_gallium, inc_gallium_aux, inc_include, inc_src, + inc_asahi, inc_virtio_gpu, inc_virtio_vdrm, + ], + dependencies : [hk_deps, idep_mesautil], + link_with : [libasahi_lib, libasahi_layout, libasahi_compiler]), + suite : ['asahi']) +endif libhk = static_library( 'hk', diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index 1e4ff1711363..28360711181f 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -407,7 +407,10 @@ stable_map_survives_application_maps(void) placed); } ASSERT_MAPPED(&mem, placed, ALLOC_SIZE); - if (memcmp(mem_map, stable, CANARY_LEN) != 0) + uint8_t readback[CANARY_LEN]; + if (!guarded_copy(readback, stable, CANARY_LEN)) + CASE_FAIL("stable BO mapping faults while checking map coherence"); + if (memcmp(mem_map, readback, CANARY_LEN) != 0) CASE_FAIL("placed mapping disagrees with the stable BO mapping"); /* Application unmap of that memory object's mapping @@ -418,7 +421,6 @@ stable_map_survives_application_maps(void) /* The stable mapping survives: same pointer, same bytes. */ if (m.bo._map != (void *)stable) CASE_FAIL("application maps replaced the stable BO mapping pointer"); - uint8_t readback[CANARY_LEN]; if (!guarded_copy(readback, stable, CANARY_LEN)) CASE_FAIL("stable BO mapping faults after application placed map + " "unmap"); From 9f8bc55f9e09dc51aa76113da82c7d9d59b03342 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:05:34 -0500 Subject: [PATCH 16/19] virtio: bound the BO host map by the requested length agx_virtio_bo_mmap ignored the length argument and always asked the host for the whole cached BO. A placed map on a BO larger than the VkDeviceMemory then maps past the address the application reserved and records the wrong _map_size, unlike the native backend's length ?: bo->size. Ask vdrm_bo_map for exactly the requested length and fall back to the BO size only for the length-0 ordinary map; the MAP_FAILED cleanup is unchanged. Expose the primitive so the placed-map regression drives the real function against a recorded vdrm stub: placed must request exactly the allocation, length 0 the whole BO, and MAP_FAILED must leave the BO slot cleared. The case is red against the pre-fix length logic and green with it. --- src/asahi/lib/agx_device_virtio.c | 13 ++- src/asahi/lib/agx_device_virtio.h | 3 + src/asahi/vulkan/tests/test-placed-map.c | 132 +++++++++++++++++++++-- 3 files changed, 132 insertions(+), 16 deletions(-) diff --git a/src/asahi/lib/agx_device_virtio.c b/src/asahi/lib/agx_device_virtio.c index 0072120b7010..a3707c85f2c8 100644 --- a/src/asahi/lib/agx_device_virtio.c +++ b/src/asahi/lib/agx_device_virtio.c @@ -190,17 +190,20 @@ agx_virtio_bo_unbind_object(struct agx_device *dev, uint32_t object_handle) return 0; } -static void +/* Real mapping primitive, non-static so the placed-map regression can drive + * it: the per-length contract it implements belongs to the ops signature. */ +void agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, - UNUSED size_t length) + size_t length) { - bo->_map_size = bo->size; - bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, bo->size, fixed_addr); + size_t map_size = length ?: bo->size; + bo->_map_size = map_size; + bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, map_size, fixed_addr); if (bo->_map == MAP_FAILED) { bo->_map = NULL; bo->_map_size = 0; fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i\n", bo->_map, - (long long)bo->size, dev->fd); + (long long)map_size, dev->fd); } } diff --git a/src/asahi/lib/agx_device_virtio.h b/src/asahi/lib/agx_device_virtio.h index 895fa311e1e5..9e7d50298745 100644 --- a/src/asahi/lib/agx_device_virtio.h +++ b/src/asahi/lib/agx_device_virtio.h @@ -11,3 +11,6 @@ int agx_virtio_simple_ioctl(struct agx_device *dev, unsigned cmd, void *_req); bool agx_virtio_open_device(struct agx_device *dev); + +void agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, + void *fixed_addr, size_t length); diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index 28360711181f..1a494b6cebae 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -24,17 +24,20 @@ * hk_memory_map_from_bo, hk_memory_unmap and hk_memory_unmap_reserve from * hk_device_memory.h operating on real struct hk_device_memory objects, plus * agx_bo_map exactly as defined in agx_device.h with the bind expression of - * hk_image_plane_bind, and the real agx_bo_unreference. A mutation of the - * shared helpers back to BO-owned application mappings therefore fails these - * cases. The only mock is the kernel mmap: mock_bo_mmap backs the BO with - * one tmpfile (ftruncate to the BO size) and hands out MAP_SHARED mappings - * of it, so two live mappings of the same BO show the same bytes, like two - * CPU mappings of one GEM object. The placed target is a PROT_NONE - * reservation that stays mapped until the MAP_FIXED placed map replaces it. - * Every resource failure FAILs the case; there are no SKIP paths. There is - * no GPU, no device node and no Vulkan entrypoint; those are covered by the - * admitted device gate (dEQP-VK host-copy coverage with a placed map after - * image bind), not here. + * hk_image_plane_bind, the real agx_bo_unreference, and the real + * agx_virtio_bo_mmap. A mutation of the shared helpers back to BO-owned + * application mappings, or of the virtio primitive away from the per-length + * ops contract, fails these cases. The mocks stand in for the kernel: the + * bo_mmap shim backs the BO with one tmpfile (ftruncate to the BO size) and + * hands out MAP_SHARED mappings of it, so two live mappings of the same BO + * show the same bytes, like two CPU mappings of one GEM object, and the + * vdrm host-side shim records what the virtio primitive asks the host to + * map. The placed target is a PROT_NONE reservation that stays mapped until + * the MAP_FIXED placed map replaces it. Every resource failure FAILs the + * case; there are no SKIP paths. There is no GPU, no device node and no + * Vulkan entrypoint; those are covered by the admitted device gate + * (dEQP-VK host-copy coverage with a placed map after image bind), not + * here. * * The stable-map case pins the decoder contract: the internal mapping must * exist, match agx_bo_map, stay pointer-stable and coherent through the @@ -54,6 +57,7 @@ #include #include +#include #include #include #include @@ -61,6 +65,10 @@ #include #include "hk_device_memory.h" +#include "virtio/vdrm/vdrm.h" + +extern void agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, + void *fixed_addr, size_t length); /* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); * a placed map covers the allocation only. Same numbers as the PR: 0x100000 @@ -107,6 +115,12 @@ struct mock_state { FILE *file; struct mock_map maps[MOCK_MAX_MAPS]; unsigned map_count; + + /* Host side of the virtio backend under test. */ + struct vdrm_device vdrm; + size_t vdrm_map_size; + void *vdrm_map_placed; + bool vdrm_fail_next; }; static void @@ -642,6 +656,101 @@ unmap_without_map_is_noop(void) return rc; } +/* The virtio mapping primitive is the one bo_mmap implementer the ops + * contract binds by length: vdrm_bo_map must receive exactly the requested + * placed length (the application reserved only that much address space) and + * record it, while a length of 0 keeps the whole cached BO. Drives the real + * agx_virtio_bo_mmap with a stubbed vdrm host side; MAP_FAILED propagation + * keeps the cleanup path covered. */ +static void * +mock_vdrm_bo_map(struct vdrm_device *vdev, uint32_t handle, size_t size, + void *placed_addr) +{ + struct mock_state *m = (struct mock_state *)((char *)vdev - + offsetof(struct mock_state, + vdrm)); + + if (m->vdrm_fail_next) + return MAP_FAILED; + + m->vdrm_map_size = size; + m->vdrm_map_placed = placed_addr; + + void *p = mmap(placed_addr, size, PROT_READ | PROT_WRITE, + MAP_SHARED | (placed_addr ? MAP_FIXED : 0), fileno(m->file), + 0); + if (p == MAP_FAILED) + return MAP_FAILED; + + mock_track(m, p, size); + return p; +} + +static int +virtio_mmap_honors_length(void) +{ + const char *name = "virtio_mmap_honors_length"; + struct mock_state m; + int rc = 1; + + if (!mock_bo_init(&m, BO_SIZE)) + CASE_FAIL("tmpfile/ftruncate for the BO failed"); + + /* The production primitive only reads dev->vdrm and dev->fd. */ + memset(&m.vdrm, 0, sizeof(m.vdrm)); + static const struct vdrm_device_funcs funcs = { + .bo_map = mock_vdrm_bo_map, + }; + m.vdrm.funcs = &funcs; + m.dev.vdrm = &m.vdrm; + m.dev.fd = fileno(m.file); + m.bo.handle = 1; + + /* Placed: the BO cache may hold a mapping up to twice the allocation, so + * the primitive must ask the host for exactly the requested length. The + * reservation extends past the placed target, so the pre-fix oversized + * MAP_FIXED lands in reserved address space instead of corrupting live + * memory and the assertions report the defect. */ + void *placed = reserve_placed_address(&m, BO_SIZE + ALLOC_SIZE); + if (placed == NULL) + CASE_FAIL("could not reserve the placed target address"); + agx_virtio_bo_mmap(&m.dev, &m.bo, placed, ALLOC_SIZE); + if (m.bo._map != placed || m.bo._map_size != ALLOC_SIZE) + CASE_FAIL("virtio placed map returned map=%p size=%zu, wanted %p+%zu", + m.bo._map, m.bo._map_size, placed, (size_t)ALLOC_SIZE); + if (m.vdrm_map_size != ALLOC_SIZE || m.vdrm_map_placed != placed) + CASE_FAIL("virtio host map requested size=%zu placed=%p, wanted " + "%u+%p", + m.vdrm_map_size, m.vdrm_map_placed, + (unsigned)ALLOC_SIZE, placed); + + /* Ordinary: a length of 0 maps and records the whole cached BO. */ + agx_virtio_bo_mmap(&m.dev, &m.bo, NULL, 0); + if (m.bo._map == NULL || m.bo._map == placed || + m.bo._map_size != m.bo.size) + CASE_FAIL("virtio ordinary map returned map=%p size=%zu, wanted the " + "whole BO", + m.bo._map, m.bo._map_size); + if (m.vdrm_map_size != m.bo.size) + CASE_FAIL("virtio host ordinary map asked for %zu, wanted %zu", + m.vdrm_map_size, m.bo.size); + munmap(m.bo._map, m.bo._map_size); + + /* MAP_FAILED from the host must leave the BO slot cleared. */ + m.vdrm_fail_next = true; + agx_virtio_bo_mmap(&m.dev, &m.bo, NULL, 0); + if (m.bo._map != NULL || m.bo._map_size != 0) + CASE_FAIL("virtio MAP_FAILED left map=%p size=%zu on the BO", + m.bo._map, m.bo._map_size); + + printf("PASS %s\n", name); + rc = 0; + +out: + mock_bo_cleanup(&m); + return rc; +} + int main(void) { @@ -651,6 +760,7 @@ main(void) failures += shared_bo_importers_map_independently(); failures += free_one_importer_survivor_survives(); failures += unmap_without_map_is_noop(); + failures += virtio_mmap_honors_length(); for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) failures += run_case(cases[i].name, cases[i].flags); From 4ca6985fb406d989dc05286af304effb577366ba Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:08:32 -0500 Subject: [PATCH 17/19] test: use the real virtio map declaration Include the production header instead of maintaining a second prototype. The registered nine-case mapping test passes after rebuilding with that declaration. --- src/asahi/vulkan/tests/test-placed-map.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index 1a494b6cebae..abddfdeae92f 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -65,10 +65,9 @@ #include #include "hk_device_memory.h" +#include "asahi/lib/agx_device_virtio.h" #include "virtio/vdrm/vdrm.h" -extern void agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, - void *fixed_addr, size_t length); /* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); * a placed map covers the allocation only. Same numbers as the PR: 0x100000 From 093714aec24529ae3b127f3b0439b4f3126923c3 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Sat, 31 Oct 2026 17:57:32 -0500 Subject: [PATCH 18/19] virtio: clear map size when the host returns NULL The real virtgpu mapper returns NULL on ioctl or mmap failure. Normalize both failure sentinels so no failed BO retains a map length. The nine-case CPU regression fails on the new NULL case before this fix and passes afterward; no VM or GPU qualification is claimed. --- src/asahi/lib/agx_device_virtio.c | 2 +- src/asahi/vulkan/tests/test-placed-map.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/asahi/lib/agx_device_virtio.c b/src/asahi/lib/agx_device_virtio.c index a3707c85f2c8..6a07d13aa5cf 100644 --- a/src/asahi/lib/agx_device_virtio.c +++ b/src/asahi/lib/agx_device_virtio.c @@ -199,7 +199,7 @@ agx_virtio_bo_mmap(struct agx_device *dev, struct agx_bo *bo, void *fixed_addr, size_t map_size = length ?: bo->size; bo->_map_size = map_size; bo->_map = vdrm_bo_map(dev->vdrm, bo->handle, map_size, fixed_addr); - if (bo->_map == MAP_FAILED) { + if (bo->_map == NULL || bo->_map == MAP_FAILED) { bo->_map = NULL; bo->_map_size = 0; fprintf(stderr, "mmap failed: result=%p size=0x%llx fd=%i\n", bo->_map, diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index abddfdeae92f..d37ab47dc3d9 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -120,6 +120,7 @@ struct mock_state { size_t vdrm_map_size; void *vdrm_map_placed; bool vdrm_fail_next; + void *vdrm_failure; }; static void @@ -659,8 +660,8 @@ unmap_without_map_is_noop(void) * contract binds by length: vdrm_bo_map must receive exactly the requested * placed length (the application reserved only that much address space) and * record it, while a length of 0 keeps the whole cached BO. Drives the real - * agx_virtio_bo_mmap with a stubbed vdrm host side; MAP_FAILED propagation - * keeps the cleanup path covered. */ + * agx_virtio_bo_mmap with a stubbed vdrm host side; NULL and MAP_FAILED + * propagation keep the cleanup path covered. */ static void * mock_vdrm_bo_map(struct vdrm_device *vdev, uint32_t handle, size_t size, void *placed_addr) @@ -670,7 +671,7 @@ mock_vdrm_bo_map(struct vdrm_device *vdev, uint32_t handle, size_t size, vdrm)); if (m->vdrm_fail_next) - return MAP_FAILED; + return m->vdrm_failure; m->vdrm_map_size = size; m->vdrm_map_placed = placed_addr; @@ -737,11 +738,19 @@ virtio_mmap_honors_length(void) /* MAP_FAILED from the host must leave the BO slot cleared. */ m.vdrm_fail_next = true; + m.vdrm_failure = MAP_FAILED; agx_virtio_bo_mmap(&m.dev, &m.bo, NULL, 0); if (m.bo._map != NULL || m.bo._map_size != 0) CASE_FAIL("virtio MAP_FAILED left map=%p size=%zu on the BO", m.bo._map, m.bo._map_size); + /* The real virtgpu backend returns NULL when its host map fails. */ + m.vdrm_failure = NULL; + agx_virtio_bo_mmap(&m.dev, &m.bo, NULL, 0); + if (m.bo._map != NULL || m.bo._map_size != 0) + CASE_FAIL("virtio NULL left map=%p size=%zu on the BO", + m.bo._map, m.bo._map_size); + printf("PASS %s\n", name); rc = 0; From d4fd1a4cb06f2f1cfbec79c07afa4b681e492988 Mon Sep 17 00:00:00 2001 From: Joshua Warren <816217+joshuaswarren@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:16:21 -0600 Subject: [PATCH 19/19] test: trim the review history from the placed-map test The header of test-placed-map.c carried review history: a GitHub discussion URL, a thread id, fixture head ids, and file:line references into the driver. It now says what the test checks and how, and names the functions instead of line numbers. The same file:line references in two code comments are gone. Only comments change, plus one failure message that quoted the thread id. Compiling the file with comments stripped gives the same code except that string. hk_placed_map_lifetime passes (1/1). --- src/asahi/vulkan/tests/test-placed-map.c | 93 +++++++++++------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/src/asahi/vulkan/tests/test-placed-map.c b/src/asahi/vulkan/tests/test-placed-map.c index d37ab47dc3d9..e99c81effd43 100644 --- a/src/asahi/vulkan/tests/test-placed-map.c +++ b/src/asahi/vulkan/tests/test-placed-map.c @@ -4,55 +4,47 @@ */ /* - * CPU-only regression for the omacom/mesa#8 review finding - * (https://github.com/omacom/mesa/pull/8#discussion_r4418383484) and its - * follow-ups: + * CPU-only regression test for per-memory application mappings. * - * A non-sparse image bind retains an internal host-copy mapping - * (hk_image_plane_bind, hk_image.c:1334: plane->map = agx_bo_map(mem->bo) + - * offset) without setting mem->map or taking a BO reference. While the - * application mapping lived on the BO, any vkMapMemory2KHR placed map, - * vkUnmapMemory2KHR (plain or VK_MEMORY_UNMAP_RESERVE_BIT_EXT) or - * vkFreeMemory of the same memory destroyed that retained pointer, and two - * memory objects sharing an imported BO shared one replaceable mapping. The - * fix gives the BO a single stable internal mapping (agx_bo_map -> - * bo->_map, read directly by decode.c and populated by wrap.c) and gives - * each VkDeviceMemory its own application mapping from the bo_mmap + * A non-sparse image bind keeps an internal host-copy mapping + * (plane->map = agx_bo_map(mem->bo) + offset in hk_image_plane_bind) without + * setting mem->map or taking a BO reference. If the application mapping lived + * on the BO, a placed vkMapMemory2KHR, a plain or VK_MEMORY_UNMAP_RESERVE_BIT_EXT + * vkUnmapMemory2KHR, or vkFreeMemory of the same memory would destroy that + * retained pointer, and two memory objects sharing an imported BO would share + * one replaceable mapping. The fix gives the BO one stable internal mapping + * (agx_bo_map -> bo->_map, read directly by decode.c and populated by wrap.c) + * and gives each VkDeviceMemory its own application mapping from the bo_mmap * primitive, dropped only by that object's unmap, reserve unmap and free. * - * This test drives the real production mapping code shared with the driver: + * The test drives the production mapping code shared with the driver: * hk_memory_map_from_bo, hk_memory_unmap and hk_memory_unmap_reserve from - * hk_device_memory.h operating on real struct hk_device_memory objects, plus - * agx_bo_map exactly as defined in agx_device.h with the bind expression of - * hk_image_plane_bind, the real agx_bo_unreference, and the real - * agx_virtio_bo_mmap. A mutation of the shared helpers back to BO-owned - * application mappings, or of the virtio primitive away from the per-length - * ops contract, fails these cases. The mocks stand in for the kernel: the - * bo_mmap shim backs the BO with one tmpfile (ftruncate to the BO size) and - * hands out MAP_SHARED mappings of it, so two live mappings of the same BO - * show the same bytes, like two CPU mappings of one GEM object, and the - * vdrm host-side shim records what the virtio primitive asks the host to + * hk_device_memory.h on real struct hk_device_memory objects, agx_bo_map as + * defined in agx_device.h with the bind expression above, the real + * agx_bo_unreference, and the real agx_virtio_bo_mmap. Mutating the shared + * helpers back to BO-owned application mappings, or the virtio primitive away + * from the per-length ops contract, fails these cases. The mocks stand in for + * the kernel: the bo_mmap shim backs the BO with one tmpfile (ftruncate to the + * BO size) and hands out MAP_SHARED mappings of it, so two live mappings of + * the same BO show the same bytes, like two CPU mappings of one GEM object. + * The vdrm host-side shim records what the virtio primitive asks the host to * map. The placed target is a PROT_NONE reservation that stays mapped until - * the MAP_FIXED placed map replaces it. Every resource failure FAILs the - * case; there are no SKIP paths. There is no GPU, no device node and no - * Vulkan entrypoint; those are covered by the admitted device gate - * (dEQP-VK host-copy coverage with a placed map after image bind), not - * here. + * the MAP_FIXED placed map replaces it. Every resource failure fails the case; + * there are no skip paths. There is no GPU, no device node and no Vulkan + * entrypoint here. * - * The stable-map case pins the decoder contract: the internal mapping must - * exist, match agx_bo_map, stay pointer-stable and coherent through the - * application map lifecycle. The two-importer and free cases pin shared-BO - * lifetime: distinct mappings per memory object, mutual content coherence, - * and a survivor whose mapping outlives the other importer's unmap or - * free. The heap accounting, ext-BO tracking and vk_device_memory_destroy - * tail of hk_FreeMemory need a live vk_device and stay behind the admitted - * device gate. + * Cases: + * - stable map: the internal mapping exists, matches agx_bo_map, stays + * pointer-stable and coherent through the application map lifecycle. + * - two importers and free: distinct mappings per memory object, mutual + * content coherence, and a survivor whose mapping outlives the other + * importer's unmap or free. + * - map before bind: the application-first order leaves the internal BO map + * independent. * - * Red was parent-observed on the pre-fix lineage (fixture heads 8af285bd30c - * and e14884a59ca: stable _map absent, importer B reusing importer A's - * placed mapping; the free case stopped at the same pointer-independence - * assertion before reaching the survivor checks). Passes with per-memory - * ownership. + * The heap accounting, ext-BO tracking and vk_device_memory_destroy tail of + * hk_FreeMemory need a live vk_device and are covered by the device gate + * (host-copy coverage with a placed map after image bind), not here. */ #include @@ -70,8 +62,7 @@ /* The BO cache can hand back a BO larger than the request (agx_bo_cache_fetch); - * a placed map covers the allocation only. Same numbers as the PR: 0x100000 - * allocation in a 0x104000 BO. */ + * a placed map covers the allocation only: a 0x100000 allocation in a 0x104000 BO. */ #define ALLOC_SIZE 0x100000 #define BO_SIZE 0x104000 @@ -83,8 +74,8 @@ #define CANARY_LEN 0x1000 -/* Case selection bits. Plain unmap (hk_device_memory.c:405) is the default - * application teardown; CASE_RESERVE uses the :394 overlay instead. */ +/* Case selection bits. Plain unmap is the default application teardown; + * CASE_RESERVE uses the reserve-unmap overlay instead. */ #define CASE_RESERVE (1u << 0) #define CASE_ALIAS (1u << 1) #define CASE_MAP_FIRST (1u << 2) @@ -225,7 +216,7 @@ fault_handler(int sig) } /* Reads through a possibly-dangling retained pointer the way the host-copy - * paths do (hk_image.c:1523). Returns false when the access faults. */ + * paths do. Returns false when the access faults. */ static bool guarded_copy(void *dst, const void *src, size_t n) { @@ -299,7 +290,7 @@ run_case(const char *name, unsigned flags) CASE_FAIL("application map populated stable BO mapping before bind"); } - /* Bind (hk_image.c:1333-1335): internal mapping retained by the image. */ + /* Bind: internal mapping retained by the image. */ void *internal = agx_bo_map(&m.bo); if (internal == NULL) CASE_FAIL("internal map returned NULL"); @@ -356,12 +347,12 @@ run_case(const char *name, unsigned flags) } ASSERT_UNMAPPED(&mem); - /* Lifetime: host-copy read through the retained pointers (hk_image.c:1523). + /* Lifetime: host-copy read through the retained pointers. * The placed map and unmap must never touch the internal mapping. */ uint8_t readback[CANARY_LEN]; if (!guarded_copy(readback, image_a, CANARY_LEN)) CASE_FAIL("host copy through the retained image mapping faults after " - "placed map + unmap (omacom/mesa#8 r4418383484)"); + "placed map + unmap"); if (memcmp(readback, pat_a, CANARY_LEN) != 0) CASE_FAIL("retained image mapping was destroyed or reused: canary " "mismatch"); @@ -531,7 +522,7 @@ shared_bo_importers_map_independently(void) /* The user-required free path: two memory objects share one imported BO; * freeing one runs the BO-relevant hk_FreeMemory sequence (placed-unmap - * condition, hk_device_memory.c:283, plus the real agx_bo_unreference) and + * condition, plus the real agx_bo_unreference) and * the survivor's mapping must stay valid and coherent. The heap accounting, * ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a * live vk_device and stay behind the admitted device gate. */