amdxdna/vxdna: support multiple virtio-gpu guest clients on the NPU#1504
Merged
Conversation
Closing a BO handle clears abo->client, while the underlying GEM object
may remain alive due to internal kernel references. As a result, code
executed after the BO handle is closed may dereference a NULL abo->client
pointer.
Remove accesses to abo->client from code paths that may execute after the
BO handle has been closed.
Fixes: d76856beb4a4 ("accel/amdxdna: Refactor GEM BO handling and add helper APIs for address retrieval")
Reviewed-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Link: https://patch.msgid.link/20260707201556.562191-1-lizhi.hou@amd.com
Signed-off-by: Wendy Liang <wendy.liang@amd.com>
A hardware context is identified to user space by its ID. Because a
process may open the device more than once, that ID must be unique
across the whole device, not just within one open file. The virtio-gpu
guest shim also derives a per-context virtio ring index from the ID as
(id - 1) % N, so device-wide uniqueness is the prerequisite that lets
the shim build that mapping.
What this patch guarantees, and what it does not:
- It guarantees the kernel hands out device-wide-unique context IDs
and, via the cyclic policy below, does not immediately reuse a
just-freed ID.
- It does NOT guarantee ring-index uniqueness. With MAX_HWCTX_ID =
1024 over N = 64 rings, IDs congruent mod 64 (e.g. 1 and 65) map to
the same ring, which can only happen once more than 64 contexts are
live. Nothing in the kernel computes (id - 1) % 64 or caps the live
context count, so the modulo mapping and the 64-live-context ceiling
are enforced by the guest shim, not by this patch; the kernel only
provides unique, slowly-recycled IDs for the shim to build on.
Split ID allocation from hwctx storage:
- ID allocation is device-wide: a struct ida on amdxdna_dev hands out
the globally-unique ID.
- Storage and lookup stay per-client: amdxdna_client.hwctx_xa maps
id -> hwctx, so a lookup can only ever return the caller's own
context.
Keeping the map per-client means a lookup never dereferences another
client's hwctx, so no ownership check is needed and a single per-client
hwctx_srcu is sufficient to guard hwctx lifetime: synchronize_srcu() on
teardown only waits on the owning client's readers, and a parked
submit/wait can only stall its own client's teardown, never another's.
Allocate the ID cyclically so a just-freed ID -- and therefore its ring
slot -- is not handed to a new context immediately, before the previous
owner's ring state is torn down. IDA has no cyclic variant (only the
XArray does, via xa_alloc_cyclic()), so keep an explicit next_hwctxid
cursor: search [cursor, MAX] first and, only on -ENOSPC, retry from
[MIN, MAX]. That retry is equivalent to [MIN, cursor) since a -ENOSPC
from the first search means [cursor, MAX] is already full; it delays
reuse but, once the space wraps, reuse is unavoidable. IDA is the
right-sized tool: the device side needs only the used/free bit (a
compact bitmap), not an id->pointer map, which lives per-client.
Both the IDA and the cursor are serialized by dev_lock, which the only
allocation site (create) and the free sites (destroy/close) already
hold, so the cursor needs no extra locking and the IDA's own internal
lock covers the bitmap.
The ID space also changes from per-open-file to device-wide, which
widens the exhaustion blast radius: previously a client could at worst
fill its own 1024-entry space, whereas now one client that creates
contexts until the shared [1, 1024] IDA is exhausted makes every other
client on the device fail with -ENOSPC. This is already bounded on aie2
(npu1/4/5/6) by the firmware context limit -- a shared device-wide
resource that caps live contexts well below 1024 -- and unchanged there.
On aie4, which has no in-kernel context-count cap, it relies on the
trusted single-guest environment (and on global memory pressure from
heavyweight context creation binding first). No per-client quota is
added here.
Signed-off-by: Wendy Liang <wendy.liang@amd.com>
wendyliang25
requested review from
houlz0507,
maxzhen and
xdavidz
as code owners
July 17, 2026 02:24
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the amdxdna/vxdna stack to support multiple concurrent virtio-gpu guest clients on the NPU by making hwctx identifiers device-wide unique and routing completions through a bounded virtio ring index. It also adds a driver-forwarded BO sync path needed for device→host read-back buffers and adjusts tests to skip native-driver-only behaviors on virtio guests.
Changes:
- Kernel: allocate hwctx IDs from a device-wide cyclic IDA (enabling multi-open) and harden GEM paths against
abo->clientbecoming NULL. - Host/guest virtio shim: fold ctx handles into a bounded virtio ring index, replace hwctx map with O(1) slot lookup, and add
SYNC_BOCCMD forwarding. - Guest-side robustness: raw-iovec fallback when userptr coalescing via
mremap(old_size=0)fails; tests skip unsupported ioctl/state operations on virtio guests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/shim_test/shim_test.cpp | Skips native-only ioctl/state tests when running against virtio-gpu guest fd. |
| src/vxdna/src/vaccel_amdxdna.h | Introduces bounded hwctx slot storage and declares new helpers/constants (incl. SYNC_BO). |
| src/vxdna/src/vaccel_amdxdna.cpp | Implements ring-index folding, slot-based hwctx management, SYNC_BO handling, and raw-iovec BO-create fallback. |
| src/vxdna/src/amdxdna_proto.h | Adds AMDXDNA_MAX_HWCTX_PER_CTX and new AMDXDNA_CCMD_SYNC_BO request/response structs. |
| src/shim/virtio/platform_virtio.h | Adds virtio platform driver sync_bo() override. |
| src/shim/virtio/platform_virtio.cpp | Routes WAIT via bounded ring index; forwards SYNC_BO CCMD to host backend. |
| drivers/accel/amdxdna/amdxdna_ubuf.c | Rejects non-contiguous VA entries without an IOMMU domain (SVA/PA modes). |
| drivers/accel/amdxdna/amdxdna_pci_drv.h | Adds device-wide IDA state for hwctx ID allocation. |
| drivers/accel/amdxdna/amdxdna_pci_drv.c | Initializes/destroys hwctx IDA and removes same-pid second-open rejection. |
| drivers/accel/amdxdna/amdxdna_gem.h | Avoids dereferencing abo->client in address helpers after close. |
| drivers/accel/amdxdna/amdxdna_gem.c | Avoids abo->client deref in vmap/HMM paths; uses device pointer instead. |
| drivers/accel/amdxdna/amdxdna_ctx.c | Implements device-wide cyclic hwctx ID allocation and frees IDs on destroy/close. |
| drivers/accel/amdxdna/aie2_message.c | Avoids cmd_abo->client deref; uses device pointer from GEM object. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maxzhen
previously approved these changes
Jul 17, 2026
wendyliang25
force-pushed
the
virtio-multi-clients
branch
from
July 17, 2026 07:22
a3b9497 to
dc932d7
Compare
…ess" This reverts commit 7efb64a. That change rejected a second open of the device from the same process because a hardware context was identified by (pid, context_id) while context_id was allocated per open file, so two opens could produce two contexts sharing the same (pid, context_id). Context IDs are now allocated from a device-wide xarray and are unique across the whole device for their lifetime, so a context is unambiguously identified by its ID regardless of how many times a process opens the device. The one-open-per-process restriction is no longer needed; drop it and restore support for multiple opens per process. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
The driver no longer rejects a second open of the device from the same process (see "Revert accel/amdxdna: reject a second device open from the same process"): context IDs are now device-wide-unique, so multiple opens per process are supported again. TEST_reject_second_open asserted the old behaviour (a second open() fails with EBUSY), so it now fails against the current driver. Remove it. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
A userptr BO is created from a table of {vaddr, len} entries whose pages
amdxdna_get_ubuf() pins and concatenates into one buffer. The BO records
only the first entry's vaddr as its user VA, so whether the entries may
be non-contiguous depends on how the device addresses the BO:
- With a kernel IOMMU domain (amdxdna_iova_on), the scattered pages are
mapped to a single contiguous device IOVA by iommu_map_sgtable(), so
the entries need not be contiguous in user VA -- the device never
sees the gaps.
- Without one (PASID/SVA, or PA mode), the device addresses the BO at
its user VA. Non-contiguous entries then produce a BO whose reported
VA does not match its later pages, and nothing else in this path
rejects it (amdxdna_dma_map_bo() runs only when PASID is off and is
type-gated).
So enforce contiguity only when there is no IOMMU domain: reject a gap
between consecutive entries with -EINVAL in that case, and leave the
IOVA path free to accept scattered entries.
Signed-off-by: Wendy Liang <wendy.liang@amd.com>
A multi-iovec resource BO is normally built by reserving one coalesced VMA and duplicating the guest slices into it with mremap(old_size=0), giving the driver a single contiguous user VA. That only works when the guest memory is shareable: mremap(old_size=0) duplicates MAP_SHARED mappings, so when QEMU is started without share=on the slices are private-anon and the mremap fails. Previously that failure threw and aborted BO creation. Instead, do not fail on the mremap error: fall back to handing the driver the raw scattered iovecs as a multi-entry va_tbl and let CREATE_BO be the arbiter. With an IOMMU (force_iova) the driver coalesces the scattered pages into one device address -- for a heap chunk into its reserved heap IOVA region -- and the ioctl succeeds; without one (SVA/PA) CREATE_BO fails and BO creation fails with it. This applies to heap chunks too: they have no host-side scattered fallback, but the driver can still place them via the IOMMU, so there is no reason to throw before CREATE_BO gets a chance. It also removes the previous post-CREATE_BO xdna_addr heuristic, which second-guessed a BO the driver had already accepted. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
The guest uses the ctx handle it gets from CREATE_CTX as the virtio ring_idx on WAIT. That handle is the driver ctx id (ctx->id), allocated by xa_alloc_cyclic over [1, MAX_CTX_ID=255], but virtio-gpu only accepts ring indices in [0, num_rings) with num_rings <= AMDXDNA_MAX_RING_NUM (64). Once the cyclic id drifts past 63, WAIT's execbuffer is rejected with -EINVAL. Fold the ctx handle into a bounded virtio ring index with hwctx_ring_idx() = ((ctx_handle - 1) % AMDXDNA_MAX_HWCTX_PER_CTX) + 1, which maps [1, MAX_CTX_ID] into [1, 32] (ring 0 stays reserved for the platform ring / AMDXDNA_INVALID_CTX_HANDLE). Key m_hwctx_table by this ring index and apply the same fold on every hwctx lookup (create/remove/config/exec/wait) and when signalling completion fences, so the ring the host signals matches the ring the guest waits on. The guest computes the identical function (shared AMDXDNA_MAX_HWCTX_PER_CTX in amdxdna_proto.h), so no extra id is exchanged. rsp.handle still returns the driver ctx id, so get_slotidx()-based correlation with firmware telemetry (ctx_map), coredump, and HW_CONTEXT_BY_ID is unchanged. create_hwctx keeps the cap of MAX_HWCTX_PER_CTX live contexts. The fold is not injective over the full handle range: two live handles congruent under the modulus collide on one ring. create_hwctx rejects the second such create with -EBUSY rather than corrupting fence routing. This is collision-free only while ctx handles stay within one modulus window, i.e. once the driver bounds ids (per-client fd + non-cyclic allocation); until then it is an interim measure. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
A WAIT hcall set the virtio ring_idx directly to the ctx handle (driver ctx id), which can exceed virtio-gpu's ring limit (num_rings <= AMDXDNA_MAX_RING_NUM) once the driver's cyclic ctx id drifts past 63, making DRM_IOCTL_VIRTGPU_EXECBUFFER fail with -EINVAL. Fold the handle into [1, AMDXDNA_MAX_HWCTX_PER_CTX] via ((ctx_handle - 1) % AMDXDNA_MAX_HWCTX_PER_CTX) + 1, matching the host's hwctx_ring_idx() so the WAIT is routed to the ring the host signals its completion fence on. Ring 0 stays reserved for the platform ring. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
Now that hwctx_ring_idx() folds the ctx handle into the dense, bounded range [1, MAX_HWCTX_PER_CTX], a hash map keyed by that index is overkill. Replace the vaccel_map with a std::array<shared_ptr, MAX_HWCTX_PER_CTX+1> indexed directly by the ring index (slot 0 unused / reserved). Benefits: - O(1) lookup with no hashing on the per-command path (exec/wait/config/ submit_fence), via a new find_hwctx() that returns a shared_ptr copy under m_hwctx_lock (same keep-alive contract as vaccel_map::lookup). - The array is both allocator and storage: a null slot is free, so the separate size cap and the insert-collision check collapse into one "slot taken -> -EBUSY" test in create_hwctx. - No per-node heap allocation; the table is one fixed ~0.5 KiB block. create_hwctx still constructs the hwctx outside the lock (the CREATE_HWCTX ioctl and polling-thread start can block) and rolls back the slot if write_rsp throws. remove_hwctx moves the entry out under the lock and lets it destruct outside, so ~vxdna_hwctx joins its polling thread without holding the lock. m_bo_table stays a vaccel_map (GEM handles are sparse). Signed-off-by: Wendy Liang <wendy.liang@amd.com>
Some BOs cannot be synced by a guest CPU cache flush alone: a debug / uc_debug BO is written by the NPU firmware, and every full-ELF workload allocates one and reads it back device->host. In the shim that read-back goes through sync_by_driver (DRM_IOCTL_AMDXDNA_SYNC_BO), which the virtio platform never implemented, so it failed with -ENOTSUP (e.g. shim_test 29 and xrt-smi validate gemm on the guest). Add an AMDXDNA_CCMD_SYNC_BO command so the guest can forward a BO sync to the host driver. The host handler validates the BO belongs to the calling context (guest handles share the single native client's GEM namespace, so ownership must be enforced here) and then issues DRM_IOCTL_AMDXDNA_SYNC_BO on the real driver fd, which does the DMA/cache maintenance and, for a ctx-assigned BO read-back, the firmware OP_SYNC_BO flush. The request carries the driver SYNC_DIRECT_* direction plus handle/offset/ size; the reply is a bare status so the guest sees ioctl failures. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
The virtio platform left sync_bo unimplemented, so it hit the base shim_not_supported_err stub (-ENOTSUP). Any BO whose sync must go through the driver (debug / uc_debug BO read-back, and thus every full-ELF workload including xrt-smi validate gemm) failed on the guest. Override platform_drv_virtio::sync_bo to forward the request to the host via AMDXDNA_CCMD_SYNC_BO: map the buffer direction to the driver's SYNC_DIRECT_* value (same as the native platform) and hcall with a response so ioctl failures propagate back to the guest. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
…uest The force-preemption and DPM/power-mode tests drive device-wide firmware state through DRM_AMDXDNA_SET_STATE (force_fine_preemption -> query::preemption, set_power_mode -> query::performance_mode). The virtio-gpu guest shim does not forward set_state, and the state it sets is device-wide and root-only, so it cannot be safely exposed per-guest. On the guest these tests failed with -ENOTSUP (or, for force preemption, a downstream counter mismatch). The AIE-debug tests are likewise unsupported on the guest: the coredump / tile-read path is not forwarded, and the AIE MEM/REG write path goes through DRM_AMDXDNA_SET_STATE. Require the native amdxdna driver for these tests so they are skipped on the QEMU guest: - "multi-command preempt full ELF io" -> dev_filter_is_aie4_or_npu4_and_amdxdna_drv (new) - "DPM noop (no QoS)" / "DPM refcount scaling" / "DPM power modes" -> dev_filter_is_npu4_and_amdxdna_drv - "get AIE coredump and check registers" / "AIE MEM read/write" / "AIE REG read/write" -> dev_filter_is_npu4_and_amdxdna_drv Signed-off-by: Wendy Liang <wendy.liang@amd.com>
TEST_query_hw_contexts, TEST_hw_context_all, TEST_query_telemetry and TEST_query_telemetry_short_buf bypass the shim and issue DRM_IOCTL_AMDXDNA_GET_INFO / GET_ARRAY directly on the accel fd (via fork_query -> open_accel_fd). On the virtio-gpu guest that fd is virtio-gpu, which does not implement the amdxdna ioctls, so the tests fail rather than exercise anything meaningful. Add dev_filter_is_aie_and_amdxdna_drv and require the native amdxdna driver for these tests so they are skipped on the QEMU guest. The other direct-ioctl users are already covered: TEST_create_free_internal_bo (get_bo_usage) uses dev_filter_is_aie2_and_amdxdna_drv, and the raw drm_version query is only reachable via the -k CLI option, not the test list. Signed-off-by: Wendy Liang <wendy.liang@amd.com>
len is a size_t, the same width as uintptr_t, so the len > numeric_limits<uintptr_t>::max() operand is always false (Coverity CID 911375, CONSTANT_EXPRESSION_RESULT). The overflow guard base > max - len still protects base + len from wrapping.
wendyliang25
force-pushed
the
virtio-multi-clients
branch
from
July 17, 2026 08:06
dc932d7 to
d986547
Compare
houlz0507
approved these changes
Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables multiple concurrent virtio-gpu guest clients (and multiple opens
Driver (drivers/accel/amdxdna)
abo->clientafter a BO handle is closed while theGEM object stays alive on internal refs (upstream fix, Lizhi Hou).
cyclic IDA so a handle is unique across the whole device (a process may
open the device more than once). Storage/lookup stay per-client, so a
single per-client SRCU still guards hwctx lifetime. The commit message
is explicit that the kernel only guarantees unique, slowly-recycled IDs
— the
(id-1) % 64ring mapping and the 64-live-context ceiling areenforced by the guest shim.
needed now that IDs are device-wide-unique; restores multi-open support.
IOMMU domain the scattered pages are coalesced to one device IOVA
(accepted); without one (SVA/PA) the device uses the user VA, so
non-contiguous entries are rejected with -EINVAL.
vxdna host backend + virtio shim (multi-context routing)
[1, MAX_HWCTX_PER_CTX]((handle-1) % N + 1) on both host and guest soWAIT is routed to the ring the host signals on, instead of using the raw
cyclic ctx id (which overflows virtio-gpu's 64-ring limit and fails
execbuffer with -EINVAL).
lookup on the hot path, slot-as-allocator.
(DRM_IOCTL_AMDXDNA_SYNC_BO) so device→host read-back BOs (debug/uc_debug,
needed by every full-ELF workload incl.
xrt-smi validate gemm) work onthe guest; the host handler enforces per-context BO ownership.
shareable (QEMU without
share=on), hand the driver the raw scatterediovecs and let CREATE_BO decide — with
force_iovathe IOMMU coalescesthem (incl. DEV_HEAP chunks into the reserved heap IOVA); otherwise
CREATE_BO fails.
Tests (test/shim_test)
where the fd is virtio-gpu and doesn't implement the amdxdna ioctls.
set_state()/AIE-debug tests on the guest (device-wide, root-onlystate not forwarded per-guest).
Cleanup
lenbound inuintptr_range_end(CoverityCID 911375).
Tests:
Test with both iova and sva mode.
Notes: test 15 about large bo, sometimes it fails due to number of buffer segments exceeds QEMU virtio gpu resource blob number of seguments limitation(16K). This failure will be fixed by increasing the limitation.
Notes:
**
df-bwfailure is due to the qemu backend number of segments per buffer limitation, the fix will be in QEMU backend** set_state() failure is expected as we don't allow guest to set state.