[PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Taimuraz Kaitmazov]

SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
only on the vmap path. An imported BO is tested for first and flushes its
whole scatterlist, so a sync costs what the BO is worth rather than what
the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
vmap path is tried first, and indexes the page-array fallback from the
requested offset.

The four before it are the ground that has to be solid first. Patch 1
refuses an I/O memory mapping, which the driver currently stores as if it
were an ordinary kernel address. Patch 2 adds a probe that does not log,
so patch 5 does not make an exporter without a vmap op print on every
ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
zero length reaching drm_clflush_virt_range(), and an offset and size
added to the BO address without an overflow check, one level above a
function that checks the same arithmetic. All four stand on their own and
can be taken separately; only patch 5 depends on them.

v1 did not reach dri-devel, so this is the first version visible there.
It is on lore via the other lists it was copied to:
https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/

Changes in v2:
 - patch 2: take the device from the GEM object rather than abo->client.
   amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
   pre-split code held across the log and the split did not.
 - new patch 3: return early from a zero-length flush.
 - new patch 4: check the sync range for overflow on a device BO.
 - patch 5: say why the persistent mapping adds no pin.

The measurements in patch 5 were taken with the equivalent change in
AMD's out-of-tree xdna-driver, where this merged as #1541. That version
and this one differ only in a page-array fallback mainline has no field
for, reached when the mapping fails and the BO is neither imported nor
shmem backed, and in the name of the mapping helper. The flush and the
helper are otherwise identical. This version is compile-tested; it has
not been booted.

Patch 1 is from inspection rather than a reproducer. The exporter I can
test against is amdgpu, and amdgpu is the case that cannot reach it: it
implements .pin, so a non peer to peer attachment like this driver's
forces the buffer to GTT before anything maps it. Reproducing it needs a
GPU whose exporter has no .pin, which I do not have paired with an NPU
here.

Taimuraz Kaitmazov (5):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
  accel/amdxdna: return early from a zero-length flush
  accel/amdxdna: check the sync range for overflow on a device BO
  accel/amdxdna: flush only the requested range in amdxdna_flush_bo

 drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 17 deletions(-)

-- 
2.55.0

[PATCH v2 1/5] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Taimuraz Kaitmazov]

amdxdna_gem_obj_vmap() accepts whatever dma_buf_vmap() returns and only
rejects a NULL vaddr. struct iosys_map is a union discriminated by
is_iomem, so an exporter that answers with an I/O mapping leaves a
void __iomem pointer in map->vaddr, and amdxdna_gem_vmap() stores it in
abo->mem.kva, which callers use as an ordinary kernel address:
amdxdna_cmd_set_error() memsets and memcpys through it.

amdxdna_drm_va_tbl takes a dmabuf_fd, so a BO of type AMDXDNA_BO_SHARE
or AMDXDNA_BO_CMD can be any exporter's buffer. Whether such a buffer is
still in a bus aperture when it is mapped depends on the exporter.
amdxdna attaches without importer ops, so an exporter that implements
.pin has it called before the mapping, and amdgpu's removes VRAM from
the allowed domains as soon as one attachment cannot do peer to peer,
which this driver's cannot; an amdgpu buffer is therefore in GTT
before any of this runs. An exporter using drm_gem_prime_dmabuf_ops
has no .pin at all, nothing moves the buffer, and drm_gem_ttm_vmap()
answers with iosys_map_set_vaddr_iomem() for a VRAM resident object.
nouveau and radeon are in that group, so an NPU paired with one of
those GPUs reaches this.

Refuse the mapping, so it never reaches a caller that cannot use it.
vmw_gem_vmap() does the same for the same reason. Unlike that one, this
path is reachable from an unprivileged ioctl, so it does not warn;
-EOPNOTSUPP is what drm_gem_vmap_locked() already returns here for an
exporter with no vmap op.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1f190b319..b66ec9e48 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
 
 	dma_resv_assert_held(obj->resv);
 
-	if (is_import_bo(abo))
+	if (is_import_bo(abo)) {
 		ret = dma_buf_vmap(abo->dma_buf, map);
-	else
+		/* Callers use mem.kva as an ordinary kernel address. */
+		if (!ret && map->is_iomem) {
+			dma_buf_vunmap(abo->dma_buf, map);
+			return -EOPNOTSUPP;
+		}
+	} else {
 		ret = drm_gem_shmem_object_vmap(obj, map);
+	}
 	if (ret)
 		return ret;
 	if (!map->vaddr)
-- 
2.55.0

[PATCH v2 2/5] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

[Taimuraz Kaitmazov]

amdxdna_gem_vmap() logs an error whenever the mapping fails, which suits
its callers: each of them treats a failure as fatal to the operation it
is performing. The next patch adds one that does not, and an exporter
that implements no vmap op fails every call without anything caching
that, so a logging probe would print on every ioctl.

Split the mapping out into __amdxdna_gem_vmap(), which returns the error,
and leave amdxdna_gem_vmap() as that plus the log. No caller changes: it
still returns the address, or NULL after logging.

The log takes the device from the GEM object rather than from abo->client.
amdxdna_gem_obj_close() clears that pointer once the last handle to the BO
is closed, which a job holding its own reference can outlive.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 30 ++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index b66ec9e48..1b8e90cd7 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -191,12 +191,8 @@ amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
 	kfree(abo);
 }
 
-/*
- * Obtains a kernel virtual address on the BO (usually of small size).
- * The mapping is established on the first call and stays valid until
- * amdxdna_gem_vunmap() is called.
- */
-void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
+/* Returns the error instead of logging it. */
+static void *__amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 {
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL);
 	int ret;
@@ -210,13 +206,29 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
 	if (!abo->mem.kva) {
 		ret = drm_gem_vmap(to_gobj(abo), &map);
 		if (ret)
-			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
-		else
-			abo->mem.kva = map.vaddr;
+			return ERR_PTR(ret);
+		abo->mem.kva = map.vaddr;
 	}
 	return abo->mem.kva;
 }
 
+/*
+ * Obtains a kernel virtual address on the BO (usually of small size).
+ * The mapping is established on the first call and stays valid until
+ * amdxdna_gem_vunmap() is called.
+ */
+void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
+{
+	void *kva = __amdxdna_gem_vmap(abo);
+
+	if (IS_ERR(kva)) {
+		XDNA_ERR(to_xdna_dev(to_gobj(abo)->dev), "Vmap bo failed, ret %ld",
+			 PTR_ERR(kva));
+		return NULL;
+	}
+	return kva;
+}
+
 /*
  * Free mapping established through amdxdna_gem_vmap()
  */
-- 
2.55.0

[PATCH v2 3/5] accel/amdxdna: return early from a zero-length flush

[Taimuraz Kaitmazov]

SYNC_BO does not constrain its size, so a request for zero bytes reaches
amdxdna_flush_bo() and, on the vmap path, calls drm_clflush_virt_range()
with a length of zero. That helper flushes the line at end - 1 to
serialise, which for a zero length is the byte in front of the region
rather than anything the caller asked to maintain.

Nothing needs flushing for an empty range, so answer before choosing a
path.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 1b8e90cd7..7df4bbb16 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1244,6 +1244,9 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
 		return -EINVAL;
 
 	size = min(abo->mem.size, end) - offset;
+	if (!size)
+		return 0;
+
 	if (is_import_bo(abo))
 		drm_clflush_sg(abo->base.sgt);
 	else if (amdxdna_gem_vmap(abo))
-- 
2.55.0

[PATCH v2 4/5] accel/amdxdna: check the sync range for overflow on a device BO

[Taimuraz Kaitmazov]

amdxdna_drm_sync_bo_ioctl() forms the range for a device BO by adding the
caller's offset and size to the BO address without checking either, while
amdxdna_flush_bo() one call down guards the same arithmetic with
check_add_overflow().

A size that wraps flush_end leaves it below the heap it is clamped
against, so every heap fails the start >= end test, and a sync that asked
for more than the address space holds reports success having flushed
nothing. Reject it instead.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index 7df4bbb16..d944f1b99 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1287,8 +1287,13 @@ int amdxdna_drm_sync_bo_ioctl(struct drm_device *dev,
 		struct amdxdna_gem_obj *heap;
 		unsigned long heap_id;
 		u64 bo_start = amdxdna_gem_dev_addr(abo);
-		u64 flush_start = bo_start + args->offset;
-		u64 flush_end = flush_start + args->size;
+		u64 flush_start, flush_end;
+
+		if (check_add_overflow(bo_start, args->offset, &flush_start) ||
+		    check_add_overflow(flush_start, args->size, &flush_end)) {
+			ret = -EINVAL;
+			goto put_obj;
+		}
 
 		xa_for_each_range(&client->dev_heap_xa, heap_id, heap,
 				  abo->heap_start_id, abo->heap_end_id) {
-- 
2.55.0

[PATCH v2 5/5] accel/amdxdna: flush only the requested range in amdxdna_flush_bo

[Taimuraz Kaitmazov]

SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours
them only on the vmap path: an imported BO is tested for first and
flushes its whole scatterlist, and the page-array fallback flushes every
page of the BO. A sync costs what the BO is worth rather than what the
caller asked to maintain.

amdxdna_gem_obj_vmap() maps an imported BO through dma_buf_vmap(), so
try the vmap path first and leave drm_clflush_sg() as the fallback for
an exporter that cannot serve one. Index the page-array fallback from
the requested offset. An imported BO now holds a kernel mapping from its
first sync until it is freed, as a shmem BO already does. That does not
pin anything the exporter was still free to move: this driver attaches
without importer ops, so the attachment is static and the exporter has
already pinned the buffer when the sg table was mapped at import.

Measured on npu4, 64 MiB BO, pinned, minimum of 50 runs: an imported BO
cost 1056 us to sync at every size from 4 KiB up, and now tracks the
driver-owned BO at 0.7 us for 4 KiB, 17 us for 1 MiB and 1056 us for the
whole BO. The driver-owned column does not move.

An earlier version walked the scatterlist a page at a time instead. It
fixed the range case but cost about 179 ns per page of barrier and call
overhead, taking the whole-BO sync from 1056 to 3989 us, so this one
reuses the mapping instead.

This does not bracket the flush with dma_buf_begin_cpu_access() and
dma_buf_end_cpu_access(). The driver has never called them, here or
anywhere else, so the omission predates this change; what changes is that
the vmap path now serves an imported BO by default, which is where the
exporter's own coherency hook would matter most. Adding the bracket is
follow-up work rather than part of this one: the calls carry a direction
but no range, so pairing them with a ranged flush wants its own reasoning
and its own measurement.

Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
---
 drivers/accel/amdxdna/amdxdna_gem.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
index d944f1b99..4430a3e3b 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -1235,6 +1235,8 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm
 
 static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
 {
+	unsigned long first, nr_pages;
+	void *kva;
 	u64 end;
 
 	if (offset >= abo->mem.size)
@@ -1247,12 +1249,16 @@ static int amdxdna_flush_bo(struct amdxdna_gem_obj *abo, u64 offset, u64 size)
 	if (!size)
 		return 0;
 
-	if (is_import_bo(abo))
+	first = offset >> PAGE_SHIFT;
+	nr_pages = (PAGE_ALIGN(offset + size) >> PAGE_SHIFT) - first;
+
+	kva = __amdxdna_gem_vmap(abo);
+	if (!IS_ERR(kva))
+		drm_clflush_virt_range(kva + offset, size);
+	else if (is_import_bo(abo))
 		drm_clflush_sg(abo->base.sgt);
-	else if (amdxdna_gem_vmap(abo))
-		drm_clflush_virt_range(amdxdna_gem_vmap(abo) + offset, size);
 	else if (abo->base.pages)
-		drm_clflush_pages(abo->base.pages, abo->mem.size >> PAGE_SHIFT);
+		drm_clflush_pages(&abo->base.pages[first], nr_pages);
 	else
 		return -EINVAL;
 
-- 
2.55.0

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Christian König]

On 8/12/26 01:13, Taimuraz Kaitmazov wrote:
> SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
> only on the vmap path. An imported BO is tested for first and flushes its
> whole scatterlist,

Absolutely clear NAK to that from a DMA-buf maintainer side.

Flushing on imported scatterlist of a DMA-buf is a really big NO-GO.

If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory.

In other words you *CAN'T* call drm_clflush_pages() on imported memory.

Regards,
Christian.

> so a sync costs what the BO is worth rather than what
> the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
> to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
> vmap path is tried first, and indexes the page-array fallback from the
> requested offset.
> 
> The four before it are the ground that has to be solid first. Patch 1
> refuses an I/O memory mapping, which the driver currently stores as if it
> were an ordinary kernel address. Patch 2 adds a probe that does not log,
> so patch 5 does not make an exporter without a vmap op print on every
> ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
> zero length reaching drm_clflush_virt_range(), and an offset and size
> added to the BO address without an overflow check, one level above a
> function that checks the same arithmetic. All four stand on their own and
> can be taken separately; only patch 5 depends on them.
> 
> v1 did not reach dri-devel, so this is the first version visible there.
> It is on lore via the other lists it was copied to:
> https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/
> 
> Changes in v2:
>  - patch 2: take the device from the GEM object rather than abo->client.
>    amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
>    pre-split code held across the log and the split did not.
>  - new patch 3: return early from a zero-length flush.
>  - new patch 4: check the sync range for overflow on a device BO.
>  - patch 5: say why the persistent mapping adds no pin.
> 
> The measurements in patch 5 were taken with the equivalent change in
> AMD's out-of-tree xdna-driver, where this merged as #1541. That version
> and this one differ only in a page-array fallback mainline has no field
> for, reached when the mapping fails and the BO is neither imported nor
> shmem backed, and in the name of the mapping helper. The flush and the
> helper are otherwise identical. This version is compile-tested; it has
> not been booted.
> 
> Patch 1 is from inspection rather than a reproducer. The exporter I can
> test against is amdgpu, and amdgpu is the case that cannot reach it: it
> implements .pin, so a non peer to peer attachment like this driver's
> forces the buffer to GTT before anything maps it. Reproducing it needs a
> GPU whose exporter has no .pin, which I do not have paired with an NPU
> here.
> 
> Taimuraz Kaitmazov (5):
>   accel/amdxdna: refuse an I/O memory mapping of an imported BO
>   accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
>   accel/amdxdna: return early from a zero-length flush
>   accel/amdxdna: check the sync range for overflow on a device BO
>   accel/amdxdna: flush only the requested range in amdxdna_flush_bo
> 
>  drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
>  1 file changed, 49 insertions(+), 17 deletions(-)
> 
> --
> 2.55.0
> 

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Lizhi Hou]

Hi Christian,

Thanks for pointing this out.

Taimuraz, this is not introduced by your patch. And the current code 
violates dma-buf protocol. It look we need to unconditionally return 
-EOPNOTSUPP for imported BO at the beginning of amdxdna_flush_bo(). 
Could you help to modify your patch 1 for this if it makes sense?


Thanks,

Lizhi

On 8/12/26 01:57, Christian König wrote:
> On 8/12/26 01:13, Taimuraz Kaitmazov wrote:
>> SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
>> only on the vmap path. An imported BO is tested for first and flushes its
>> whole scatterlist,
> Absolutely clear NAK to that from a DMA-buf maintainer side.
>
> Flushing on imported scatterlist of a DMA-buf is a really big NO-GO.
>
> If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory.
>
> In other words you *CAN'T* call drm_clflush_pages() on imported memory.
>
> Regards,
> Christian.
>
>> so a sync costs what the BO is worth rather than what
>> the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
>> to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
>> vmap path is tried first, and indexes the page-array fallback from the
>> requested offset.
>>
>> The four before it are the ground that has to be solid first. Patch 1
>> refuses an I/O memory mapping, which the driver currently stores as if it
>> were an ordinary kernel address. Patch 2 adds a probe that does not log,
>> so patch 5 does not make an exporter without a vmap op print on every
>> ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
>> zero length reaching drm_clflush_virt_range(), and an offset and size
>> added to the BO address without an overflow check, one level above a
>> function that checks the same arithmetic. All four stand on their own and
>> can be taken separately; only patch 5 depends on them.
>>
>> v1 did not reach dri-devel, so this is the first version visible there.
>> It is on lore via the other lists it was copied to:
>> https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/
>>
>> Changes in v2:
>>   - patch 2: take the device from the GEM object rather than abo->client.
>>     amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
>>     pre-split code held across the log and the split did not.
>>   - new patch 3: return early from a zero-length flush.
>>   - new patch 4: check the sync range for overflow on a device BO.
>>   - patch 5: say why the persistent mapping adds no pin.
>>
>> The measurements in patch 5 were taken with the equivalent change in
>> AMD's out-of-tree xdna-driver, where this merged as #1541. That version
>> and this one differ only in a page-array fallback mainline has no field
>> for, reached when the mapping fails and the BO is neither imported nor
>> shmem backed, and in the name of the mapping helper. The flush and the
>> helper are otherwise identical. This version is compile-tested; it has
>> not been booted.
>>
>> Patch 1 is from inspection rather than a reproducer. The exporter I can
>> test against is amdgpu, and amdgpu is the case that cannot reach it: it
>> implements .pin, so a non peer to peer attachment like this driver's
>> forces the buffer to GTT before anything maps it. Reproducing it needs a
>> GPU whose exporter has no .pin, which I do not have paired with an NPU
>> here.
>>
>> Taimuraz Kaitmazov (5):
>>    accel/amdxdna: refuse an I/O memory mapping of an imported BO
>>    accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
>>    accel/amdxdna: return early from a zero-length flush
>>    accel/amdxdna: check the sync range for overflow on a device BO
>>    accel/amdxdna: flush only the requested range in amdxdna_flush_bo
>>
>>   drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
>>   1 file changed, 49 insertions(+), 17 deletions(-)
>>
>> --
>> 2.55.0
>>

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Christian König]

Hi Lizhi,

yeah that sounds reasonable.

An alternative would be to use DMA_BUF_IOCTL_SYNC from userspace, but that is usually only for the exporter to implement clflush or similar actions. As importer you need to be able to take the data as it is.

We have discussed before if that shouldn't be changed somehow, but so far didn't settled on an interface.

Question is why do you need clflush in the first place? The NPU is a PCIe device, isn't it? And so it should be using cache coherent memory accesses.

Regards,
Christian.

On 8/12/26 17:45, Lizhi Hou wrote:
> Hi Christian,
> 
> Thanks for pointing this out.
> 
> Taimuraz, this is not introduced by your patch. And the current code violates dma-buf protocol. It look we need to unconditionally return -EOPNOTSUPP for imported BO at the beginning of amdxdna_flush_bo(). Could you help to modify your patch 1 for this if it makes sense?
> 
> 
> Thanks,
> 
> Lizhi
> 
> On 8/12/26 01:57, Christian König wrote:
>> On 8/12/26 01:13, Taimuraz Kaitmazov wrote:
>>> SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
>>> only on the vmap path. An imported BO is tested for first and flushes its
>>> whole scatterlist,
>> Absolutely clear NAK to that from a DMA-buf maintainer side.
>>
>> Flushing on imported scatterlist of a DMA-buf is a really big NO-GO.
>>
>> If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory.
>>
>> In other words you *CAN'T* call drm_clflush_pages() on imported memory.
>>
>> Regards,
>> Christian.
>>
>>> so a sync costs what the BO is worth rather than what
>>> the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
>>> to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
>>> vmap path is tried first, and indexes the page-array fallback from the
>>> requested offset.
>>>
>>> The four before it are the ground that has to be solid first. Patch 1
>>> refuses an I/O memory mapping, which the driver currently stores as if it
>>> were an ordinary kernel address. Patch 2 adds a probe that does not log,
>>> so patch 5 does not make an exporter without a vmap op print on every
>>> ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
>>> zero length reaching drm_clflush_virt_range(), and an offset and size
>>> added to the BO address without an overflow check, one level above a
>>> function that checks the same arithmetic. All four stand on their own and
>>> can be taken separately; only patch 5 depends on them.
>>>
>>> v1 did not reach dri-devel, so this is the first version visible there.
>>> It is on lore via the other lists it was copied to:
>>> https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/
>>>
>>> Changes in v2:
>>>   - patch 2: take the device from the GEM object rather than abo->client.
>>>     amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
>>>     pre-split code held across the log and the split did not.
>>>   - new patch 3: return early from a zero-length flush.
>>>   - new patch 4: check the sync range for overflow on a device BO.
>>>   - patch 5: say why the persistent mapping adds no pin.
>>>
>>> The measurements in patch 5 were taken with the equivalent change in
>>> AMD's out-of-tree xdna-driver, where this merged as #1541. That version
>>> and this one differ only in a page-array fallback mainline has no field
>>> for, reached when the mapping fails and the BO is neither imported nor
>>> shmem backed, and in the name of the mapping helper. The flush and the
>>> helper are otherwise identical. This version is compile-tested; it has
>>> not been booted.
>>>
>>> Patch 1 is from inspection rather than a reproducer. The exporter I can
>>> test against is amdgpu, and amdgpu is the case that cannot reach it: it
>>> implements .pin, so a non peer to peer attachment like this driver's
>>> forces the buffer to GTT before anything maps it. Reproducing it needs a
>>> GPU whose exporter has no .pin, which I do not have paired with an NPU
>>> here.
>>>
>>> Taimuraz Kaitmazov (5):
>>>    accel/amdxdna: refuse an I/O memory mapping of an imported BO
>>>    accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
>>>    accel/amdxdna: return early from a zero-length flush
>>>    accel/amdxdna: check the sync range for overflow on a device BO
>>>    accel/amdxdna: flush only the requested range in amdxdna_flush_bo
>>>
>>>   drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
>>>   1 file changed, 49 insertions(+), 17 deletions(-)
>>>
>>> -- 
>>> 2.55.0
>>>

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Lizhi Hou]

On 8/13/26 00:44, Christian König wrote:
> Hi Lizhi,
>
> yeah that sounds reasonable.
Thanks. :)
>
> An alternative would be to use DMA_BUF_IOCTL_SYNC from userspace, but that is usually only for the exporter to implement clflush or similar actions. As importer you need to be able to take the data as it is.
>
> We have discussed before if that shouldn't be changed somehow, but so far didn't settled on an interface.
>
> Question is why do you need clflush in the first place? The NPU is a PCIe device, isn't it? And so it should be using cache coherent memory accesses.

I do not know the hardware detail. The legacy NPU device is not cache 
coherent. And the next generation (aie4) devices will be cache coherent.


Lizhi

>
> Regards,
> Christian.
>
> On 8/12/26 17:45, Lizhi Hou wrote:
>> Hi Christian,
>>
>> Thanks for pointing this out.
>>
>> Taimuraz, this is not introduced by your patch. And the current code violates dma-buf protocol. It look we need to unconditionally return -EOPNOTSUPP for imported BO at the beginning of amdxdna_flush_bo(). Could you help to modify your patch 1 for this if it makes sense?
>>
>>
>> Thanks,
>>
>> Lizhi
>>
>> On 8/12/26 01:57, Christian König wrote:
>>> On 8/12/26 01:13, Taimuraz Kaitmazov wrote:
>>>> SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
>>>> only on the vmap path. An imported BO is tested for first and flushes its
>>>> whole scatterlist,
>>> Absolutely clear NAK to that from a DMA-buf maintainer side.
>>>
>>> Flushing on imported scatterlist of a DMA-buf is a really big NO-GO.
>>>
>>> If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory.
>>>
>>> In other words you *CAN'T* call drm_clflush_pages() on imported memory.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> so a sync costs what the BO is worth rather than what
>>>> the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
>>>> to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
>>>> vmap path is tried first, and indexes the page-array fallback from the
>>>> requested offset.
>>>>
>>>> The four before it are the ground that has to be solid first. Patch 1
>>>> refuses an I/O memory mapping, which the driver currently stores as if it
>>>> were an ordinary kernel address. Patch 2 adds a probe that does not log,
>>>> so patch 5 does not make an exporter without a vmap op print on every
>>>> ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
>>>> zero length reaching drm_clflush_virt_range(), and an offset and size
>>>> added to the BO address without an overflow check, one level above a
>>>> function that checks the same arithmetic. All four stand on their own and
>>>> can be taken separately; only patch 5 depends on them.
>>>>
>>>> v1 did not reach dri-devel, so this is the first version visible there.
>>>> It is on lore via the other lists it was copied to:
>>>> https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/
>>>>
>>>> Changes in v2:
>>>>    - patch 2: take the device from the GEM object rather than abo->client.
>>>>      amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
>>>>      pre-split code held across the log and the split did not.
>>>>    - new patch 3: return early from a zero-length flush.
>>>>    - new patch 4: check the sync range for overflow on a device BO.
>>>>    - patch 5: say why the persistent mapping adds no pin.
>>>>
>>>> The measurements in patch 5 were taken with the equivalent change in
>>>> AMD's out-of-tree xdna-driver, where this merged as #1541. That version
>>>> and this one differ only in a page-array fallback mainline has no field
>>>> for, reached when the mapping fails and the BO is neither imported nor
>>>> shmem backed, and in the name of the mapping helper. The flush and the
>>>> helper are otherwise identical. This version is compile-tested; it has
>>>> not been booted.
>>>>
>>>> Patch 1 is from inspection rather than a reproducer. The exporter I can
>>>> test against is amdgpu, and amdgpu is the case that cannot reach it: it
>>>> implements .pin, so a non peer to peer attachment like this driver's
>>>> forces the buffer to GTT before anything maps it. Reproducing it needs a
>>>> GPU whose exporter has no .pin, which I do not have paired with an NPU
>>>> here.
>>>>
>>>> Taimuraz Kaitmazov (5):
>>>>     accel/amdxdna: refuse an I/O memory mapping of an imported BO
>>>>     accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
>>>>     accel/amdxdna: return early from a zero-length flush
>>>>     accel/amdxdna: check the sync range for overflow on a device BO
>>>>     accel/amdxdna: flush only the requested range in amdxdna_flush_bo
>>>>
>>>>    drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
>>>>    1 file changed, 49 insertions(+), 17 deletions(-)
>>>>
>>>> -- 
>>>> 2.55.0
>>>>

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Taimuraz Kaitmazov]

Resending: my earlier reply does not appear on the lists, so I assume it
did not reach you either.

On 8/13/26 20:08, Lizhi Hou wrote:
> The legacy NPU device is not cache coherent. And the next generation
> (aie4) devices will be cache coherent.

Confirming that with numbers, since I had measured it before your reply
landed. On npu4, without a flush the CPU reads what the buffer held
before the NPU wrote it, and the NPU reads what DRAM held before the CPU
wrote it. Both reproduce on all 20 runs, and the stale read is most of
the buffer, not a stray line: 3931 of 4096 values on average.

Good to know aie4 is coherent -- that makes anything we do here a
legacy-only concern.

On 8/13/26 09:44, Christian König wrote:
> An alternative would be to use DMA_BUF_IOCTL_SYNC from userspace

Tried it against amdgpu, imported into amdxdna: stale on all 20 runs, no
better than no sync at all. SYNC_BO on the same buffer is clean on all
20.

Which leaves me no legal way to import a buffer the CPU also reads. Is
there one I'm missing, or should a device like this just not import?

On 8/12/26 17:45, Lizhi Hou wrote:
> we need to unconditionally return -EOPNOTSUPP for imported BO

is_import_bo() also covers ubuf and cbuf, so that stops maintaining our
own userptr and carve-out BOs too. They take that arm today: on a 64 MiB
userptr BO a 4 KiB sync and a full sync both cost 659 us, so the range
is already being ignored there.

Keying on dma_buf->ops instead would confine it to foreign buffers.
Either is fine by me, tell me which you want.

Separately: XRT's buffer::sync() clflushes in userspace unless
Debug.force_driver_sync is set, so the stack does this to foreign
dma-bufs whatever the driver does. And when the ioctl is used, a
FROM_DEVICE sync returns -EINVAL after the flush has already run, out of
amdxdna_hwctx_sync_debug_bo() when the BO has no assigned hwctx. Happy
to send that as its own patch; the helper has one caller, so returning 0
there is the obvious shape unless you want it done elsewhere.

v3 is sent: patches 1, 3 and 4 only. Patch 5 is dropped, and 2 with it
since it only serves 5. I have not tested 5 on a matching tree and its
numbers came from the foreign import case.

Taimuraz

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Alex Deucher]

On Thu, Aug 13, 2026 at 2:45 PM Lizhi Hou <lizhi.hou@amd.com> wrote:
>
>
> On 8/13/26 00:44, Christian König wrote:
> > Hi Lizhi,
> >
> > yeah that sounds reasonable.
> Thanks. :)
> >
> > An alternative would be to use DMA_BUF_IOCTL_SYNC from userspace, but that is usually only for the exporter to implement clflush or similar actions. As importer you need to be able to take the data as it is.
> >
> > We have discussed before if that shouldn't be changed somehow, but so far didn't settled on an interface.
> >
> > Question is why do you need clflush in the first place? The NPU is a PCIe device, isn't it? And so it should be using cache coherent memory accesses.
>
> I do not know the hardware detail. The legacy NPU device is not cache
> coherent. And the next generation (aie4) devices will be cache coherent.
>

The CPU wouldn't be coherent with device caches, but the device should
be coherent with the CPU's caches.  I.e., PCIe transactions from the
device should snoop the CPU's cache.  Otherwise, you couldn't use CPU
cached memory for DMA.

Alex

>
> Lizhi
>
> >
> > Regards,
> > Christian.
> >
> > On 8/12/26 17:45, Lizhi Hou wrote:
> >> Hi Christian,
> >>
> >> Thanks for pointing this out.
> >>
> >> Taimuraz, this is not introduced by your patch. And the current code violates dma-buf protocol. It look we need to unconditionally return -EOPNOTSUPP for imported BO at the beginning of amdxdna_flush_bo(). Could you help to modify your patch 1 for this if it makes sense?
> >>
> >>
> >> Thanks,
> >>
> >> Lizhi
> >>
> >> On 8/12/26 01:57, Christian König wrote:
> >>> On 8/12/26 01:13, Taimuraz Kaitmazov wrote:
> >>>> SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them
> >>>> only on the vmap path. An imported BO is tested for first and flushes its
> >>>> whole scatterlist,
> >>> Absolutely clear NAK to that from a DMA-buf maintainer side.
> >>>
> >>> Flushing on imported scatterlist of a DMA-buf is a really big NO-GO.
> >>>
> >>> If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory.
> >>>
> >>> In other words you *CAN'T* call drm_clflush_pages() on imported memory.
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>> so a sync costs what the BO is worth rather than what
> >>>> the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us
> >>>> to sync at every size from 4 KiB up. Patch 5 reorders the arms so the
> >>>> vmap path is tried first, and indexes the page-array fallback from the
> >>>> requested offset.
> >>>>
> >>>> The four before it are the ground that has to be solid first. Patch 1
> >>>> refuses an I/O memory mapping, which the driver currently stores as if it
> >>>> were an ordinary kernel address. Patch 2 adds a probe that does not log,
> >>>> so patch 5 does not make an exporter without a vmap op print on every
> >>>> ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a
> >>>> zero length reaching drm_clflush_virt_range(), and an offset and size
> >>>> added to the BO address without an overflow check, one level above a
> >>>> function that checks the same arithmetic. All four stand on their own and
> >>>> can be taken separately; only patch 5 depends on them.
> >>>>
> >>>> v1 did not reach dri-devel, so this is the first version visible there.
> >>>> It is on lore via the other lists it was copied to:
> >>>> https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/
> >>>>
> >>>> Changes in v2:
> >>>>    - patch 2: take the device from the GEM object rather than abo->client.
> >>>>      amdxdna_gem_obj_close() clears that pointer under abo->lock, which the
> >>>>      pre-split code held across the log and the split did not.
> >>>>    - new patch 3: return early from a zero-length flush.
> >>>>    - new patch 4: check the sync range for overflow on a device BO.
> >>>>    - patch 5: say why the persistent mapping adds no pin.
> >>>>
> >>>> The measurements in patch 5 were taken with the equivalent change in
> >>>> AMD's out-of-tree xdna-driver, where this merged as #1541. That version
> >>>> and this one differ only in a page-array fallback mainline has no field
> >>>> for, reached when the mapping fails and the BO is neither imported nor
> >>>> shmem backed, and in the name of the mapping helper. The flush and the
> >>>> helper are otherwise identical. This version is compile-tested; it has
> >>>> not been booted.
> >>>>
> >>>> Patch 1 is from inspection rather than a reproducer. The exporter I can
> >>>> test against is amdgpu, and amdgpu is the case that cannot reach it: it
> >>>> implements .pin, so a non peer to peer attachment like this driver's
> >>>> forces the buffer to GTT before anything maps it. Reproducing it needs a
> >>>> GPU whose exporter has no .pin, which I do not have paired with an NPU
> >>>> here.
> >>>>
> >>>> Taimuraz Kaitmazov (5):
> >>>>     accel/amdxdna: refuse an I/O memory mapping of an imported BO
> >>>>     accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()
> >>>>     accel/amdxdna: return early from a zero-length flush
> >>>>     accel/amdxdna: check the sync range for overflow on a device BO
> >>>>     accel/amdxdna: flush only the requested range in amdxdna_flush_bo
> >>>>
> >>>>    drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++--------
> >>>>    1 file changed, 49 insertions(+), 17 deletions(-)
> >>>>
> >>>> --
> >>>> 2.55.0
> >>>>

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Lizhi Hou]

On 8/13/26 11:29, Taimuraz Kaitmazov wrote:
> Resending: my earlier reply does not appear on the lists, so I assume it
> did not reach you either.
>
> On 8/13/26 20:08, Lizhi Hou wrote:
>> The legacy NPU device is not cache coherent. And the next generation
>> (aie4) devices will be cache coherent.
> Confirming that with numbers, since I had measured it before your reply
> landed. On npu4, without a flush the CPU reads what the buffer held
> before the NPU wrote it, and the NPU reads what DRAM held before the CPU
> wrote it. Both reproduce on all 20 runs, and the stale read is most of
> the buffer, not a stray line: 3931 of 4096 values on average.
>
> Good to know aie4 is coherent -- that makes anything we do here a
> legacy-only concern.
>
> On 8/13/26 09:44, Christian König wrote:
>> An alternative would be to use DMA_BUF_IOCTL_SYNC from userspace
> Tried it against amdgpu, imported into amdxdna: stale on all 20 runs, no
> better than no sync at all. SYNC_BO on the same buffer is clean on all
> 20.
>
> Which leaves me no legal way to import a buffer the CPU also reads. Is
> there one I'm missing, or should a device like this just not import?
>
> On 8/12/26 17:45, Lizhi Hou wrote:
>> we need to unconditionally return -EOPNOTSUPP for imported BO
> is_import_bo() also covers ubuf and cbuf, so that stops maintaining our
> own userptr and carve-out BOs too. They take that arm today: on a 64 MiB
> userptr BO a 4 KiB sync and a full sync both cost 659 us, so the range
> is already being ignored there.

I am working on removing the dma-buf part for ubuf BO because that is 
also not a good usage of dma-buf. So the ubuf will be a object soon. 
cbuf is mainly for debug and is disabled by default.


Lizhi

>
> Keying on dma_buf->ops instead would confine it to foreign buffers.
> Either is fine by me, tell me which you want.
>
> Separately: XRT's buffer::sync() clflushes in userspace unless
> Debug.force_driver_sync is set, so the stack does this to foreign
> dma-bufs whatever the driver does. And when the ioctl is used, a
> FROM_DEVICE sync returns -EINVAL after the flush has already run, out of
> amdxdna_hwctx_sync_debug_bo() when the BO has no assigned hwctx. Happy
> to send that as its own patch; the helper has one caller, so returning 0
> there is the obvious shape unless you want it done elsewhere.
>
> v3 is sent: patches 1, 3 and 4 only. Patch 5 is dropped, and 2 with it
> since it only serves 5. I have not tested 5 on a matching tree and its
> numbers came from the foreign import case.
>
> Taimuraz
>
>

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Taimuraz Kaitmazov]

(Resending to the list -- my earlier copy went to Alex alone.)

On 8/13/26 21:54, Alex Deucher wrote:
> PCIe transactions from the device should snoop the CPU's cache.

That is what I assumed too, but it is not what I measure on npu4. Leaving a new input dirty in the CPU and running, the array computes on the previous contents, each out of 20 runs. Flushing that same input and running again, it sees it - so the data does reach the buffer, and something is not picking up the dirty line.

What I cannot square is that ubuf maps with dma_map_sgtable() and nothing else, and on x86 the DMA API no-ops the syncs, so that path looks correct only because SYNC_BO clflushes on top of it.

Is the array expected to snoop on this part? Happy to run whatever would settle it, or to be told what I am measuring wrong.

Taimuraz