Claude review: accel/amdxdna: Refactor GEM BO handling and add helper APIs for address retrieval

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. Race condition in `amdxdna_gem_vmap()` (amdxdna_gem.c)**

The double-checked locking pattern has a data race. The first read of `abo->mem.kva` is outside the lock and without any memory barrier:

```c
void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
{
	struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL);
	int ret;

	if (abo->mem.kva)
		return abo->mem.kva;

	/* The first call to get the kva, taking slow path. */
	guard(mutex)(&abo->lock);

	if (!abo->mem.kva) {
		ret = drm_gem_vmap(to_gobj(abo), &map);
		...
		else
			abo->mem.kva = map.vaddr;
	}
	return abo->mem.kva;
}
```

In the kernel, a plain load of `abo->mem.kva` from one thread while another thread stores under the lock is a data race per KCSAN. Use `READ_ONCE`/`WRITE_ONCE` or `smp_load_acquire`/`smp_store_release` to make this correct. The inner store should use `smp_store_release(&abo->mem.kva, map.vaddr)` and the outer check should use `smp_load_acquire(&abo->mem.kva)`.

**2. `amdxdna_gem_vmap()` called in inline hot-path accessors (amdxdna_ctx.h)**

`amdxdna_cmd_get_op()`, `amdxdna_cmd_set_state()`, and `amdxdna_cmd_get_state()` are small inline helpers that previously did a direct pointer dereference of `abo->mem.kva`. They now call `amdxdna_gem_vmap()` which takes a mutex on the first call and calls `drm_gem_vmap`. These functions are called in the job submission and completion hot paths. While functional, this is a performance regression hiding behind "no functional change." These BOs should already be vmapped by the time these helpers run. Consider asserting rather than silently handling NULL.

**3. Double call to `amdxdna_gem_vmap()` in sync ioctl (amdxdna_gem.c)**

```c
	else if (amdxdna_gem_vmap(abo))
		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + args->offset, args->size);
```

This calls `amdxdna_gem_vmap()` twice — once for the condition check, once for the argument. While it will return the cached pointer on the second call, this is wasteful and poor style. Use a local variable.

**4. Removal of `.close` callback and IOMMU unmap asymmetry (amdxdna_gem.c)**

The `.open` callback now does `amdxdna_iommu_map_bo()` but there's no corresponding `.close` callback. The IOMMU unmap is instead done in `amdxdna_gem_obj_free()`:

```c
	if (amdxdna_iova_on(xdna))
		amdxdna_iommu_unmap_bo(xdna, abo);
```

The old code had a ref-counted `.open`/`.close` pair. With the old DRM gem_open/gem_close semantics, `.open` is called for each `drm_file` that gains a handle. If the same BO is opened by multiple files (via prime import or handle duplication), `.open` will be called multiple times but `amdxdna_iommu_map_bo` will only unmap once in `.free`. This could lead to double-mapping or leaked IOMMU mappings depending on `amdxdna_iommu_map_bo`'s idempotency. The refcount removal needs justification.

**5. Lost CMD BO size validation (amdxdna_gem.c)**

The old code had:
```c
	if (args->size < sizeof(struct amdxdna_cmd)) {
		XDNA_DBG(xdna, "Command BO size 0x%llx too small", args->size);
		return ERR_PTR(-EINVAL);
	}
```

And in `amdxdna_gem_get_obj`:
```c
	if (abo->mem.size > SZ_32K) {
		XDNA_ERR(xdna, "Cmd bo is too big %ld", abo->mem.size);
		goto put_obj;
	}
```

Both are removed. Userspace can now create an SHMEM BO of any size and use it as a command buffer. The minimum size check prevented out-of-bounds reads in `amdxdna_cmd_get_payload()` and friends. The 32K upper bound prevented excessive kernel vmaps. These are security-relevant validations that should be preserved somewhere.

**6. DEV_HEAP size validation changed semantics (amdxdna_gem.c)**

Old code:
```c
	if (args->size > xdna->dev_info->dev_mem_size) {
```

New code:
```c
	if (!args->size || !IS_ALIGNED(args->size, xdna->dev_info->dev_mem_size)) {
```

The old code checked that the heap doesn't exceed device memory. The new code requires the size to be an exact multiple of `dev_mem_size`. This is a behavioral change — previously a heap of `dev_mem_size / 2` was valid; now it's not. This seems intentional but contradicts "no functional change."

**7. `amdxdna_gem_vmap()` doesn't handle imported BOs (amdxdna_gem.c)**

The old `amdxdna_gem_obj_vmap()` handled both native and imported BOs:
```c
	if (is_import_bo(abo))
		ret = dma_buf_vmap_unlocked(abo->dma_buf, &map);
	else
		ret = drm_gem_vmap(to_gobj(abo), &map);
```

The new `amdxdna_gem_vmap()` only calls `drm_gem_vmap()`. For imported BOs, `drm_gem_vmap` would need the object's `.vmap` callback, which is set from the exporter's `dma_buf_ops`. This path may or may not work — it depends on whether `drm_gem_vmap` correctly delegates to the DMA-BUF layer. If it doesn't, vmapping imported BOs will silently fail. The old code explicitly distinguished the two paths for a reason.

Similarly, `amdxdna_gem_vunmap()` only calls `drm_gem_vunmap()` but the old `amdxdna_gem_obj_vunmap()` called `dma_buf_vunmap_unlocked()` for imported BOs.

**8. ubuf always DMA-mapped now (amdxdna_ubuf.c)**

The patch removes the `AMDXDNA_UBUF_FLAG_MAP_DMA` flag and unconditionally calls `dma_map_sgtable()` in `amdxdna_ubuf_map()`. Previously only CMD BOs had their ubufs DMA-mapped. This is a functional change — non-CMD ubufs that were previously not DMA-mapped will now be mapped. This could have performance or correctness implications depending on how those BOs are used.

**9. `abo->client` set in `.open` callback may be too late (amdxdna_gem.c)**

```c
static int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *filp)
{
	...
	if (!abo->client)
		abo->client = filp->driver_priv;
```

For SHARE BOs created through `amdxdna_drm_create_share_bo()`, `abo->client` is never set in the creation path (unlike the old code which did `abo->client = client`). It's deferred to `.open`. But `amdxdna_gem_vmap()` accesses `abo->client->xdna` for error logging — if vmap is called before open (which could happen during creation), this will NULL-deref.

**10. UAPI: `AMDXDNA_BO_SHMEM` and `AMDXDNA_BO_SHARE` aliased (amdxdna_accel.h)**

```c
	AMDXDNA_BO_SHMEM = 1, /* Be compatible with legacy application code. */
	AMDXDNA_BO_SHARE = 1,
```

Having two enum values with the same underlying value is valid C but unusual in kernel UAPI headers. The comment says "legacy application code" but this driver is quite new. If SHMEM is truly being removed, a deprecation notice or compat note in the UAPI header doc comment would be helpful. Also, the kernel code now uses `AMDXDNA_BO_SHARE` internally but accepts `AMDXDNA_BO_CMD` (value 4) via `fallthrough` in the switch — consider adding a comment in the UAPI explaining that CMD is now treated as SHARE.

**11. `amdxdna_gem_dev_obj_vmap` doesn't lock (amdxdna_gem.c)**

```c
static int amdxdna_gem_dev_obj_vmap(struct drm_gem_object *obj, struct iosys_map *map)
{
	struct amdxdna_gem_obj *abo = to_xdna_obj(obj);
	void *base = amdxdna_gem_vmap(abo->client->dev_heap);
	u64 offset = amdxdna_dev_bo_offset(abo);
	...
}
```

This accesses `abo->client->dev_heap` without holding any lock. If the dev_heap could be torn down concurrently (e.g., userspace closes the heap handle while another thread vmaps a DEV BO), this could race. The old code didn't have this path at all since DEV BOs computed their KVA from the heap's KVA at allocation time.

---
Generated by Claude Code Patch Reviewer