[PATCH 1/2] drm/amdgpu: fix KFDMemoryTest.PtraceAccessInvisibleVram fail on SRIOV

[PATCH 1/2] drm/amdgpu: fix KFDMemoryTest.PtraceAccessInvisibleVram fail on SRIOV

[Samuel Zhang]

Ptrace access VRAM bo will first try sdma access in
amdgpu_ttm_access_memory_sdma(), if fails, it will fallback to mmio
access.

Since ptrace only access 8 bytes at a time and
amdgpu_ttm_access_memory_sdma() only allow PAGE_SIZE bytes access,
it returns fail.
On SRIOV, mmio access will also fail as MM_INDEX/MM_DATA register write
is blocked for security reasons.

The fix is just change len check in amdgpu_ttm_access_memory_sdma() so
that len in (0, PAGE_SIZE] are allowed. This will not only fix the ptrace
test case on SRIOV, but also improve the access performance when the
access length is < PAGE_SIZE.
len > PAGE_SIZE case support is not needed as larger size will be break
into chunks of PAGE_SIZE len max in mem_rw().

Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 5daa05e23ddf..a6e90eada367 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1486,7 +1486,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
 	unsigned int num_dw;
 	int r, idx;
 
-	if (len != PAGE_SIZE)
+	if (len > PAGE_SIZE)
 		return -EINVAL;
 
 	if (!adev->mman.sdma_access_ptr)
@@ -1514,7 +1514,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
 		swap(src_addr, dst_addr);
 
 	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
-				PAGE_SIZE, 0);
+				len, 0);
 
 	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
 	WARN_ON(job->ibs[0].length_dw > num_dw);
-- 
2.25.1

[PATCH 2/2] drm/amdgpu: fix incomplete access issue in amdgpu_ttm_access_memory_sdma()

[Samuel Zhang]

The requested access range may be across 2 adjacent buddy blocks of a
BO. In this case, it needs to issue 2 sdma copy commands to fully access
the data range. But current implementation only issue 1 sdma copy
command and result in incomplete access.

The fix is to loop the res cursor when emitting copy commands so that
multiple(2) copy commands got issued when necessary.

Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 26 ++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index a6e90eada367..c423574acd5c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1484,7 +1484,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
 	struct dma_fence *fence;
 	uint64_t src_addr, dst_addr;
 	unsigned int num_dw;
-	int r, idx;
+	int r, idx, count = 0;
 
 	if (len > PAGE_SIZE)
 		return -EINVAL;
@@ -1498,7 +1498,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
 	if (write)
 		memcpy(adev->mman.sdma_access_ptr, buf, len);
 
-	num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw, 8);
+	num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw * 2, 8);
 	r = amdgpu_job_alloc_with_ib(adev, &adev->mman.high_pr,
 				     AMDGPU_FENCE_OWNER_UNDEFINED,
 				     num_dw * 4, AMDGPU_IB_POOL_DELAYED,
@@ -1507,15 +1507,19 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
 		goto out;
 
 	amdgpu_res_first(abo->tbo.resource, offset, len, &src_mm);
-	src_addr = amdgpu_ttm_domain_start(adev, bo->resource->mem_type) +
-		src_mm.start;
-	dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo);
-	if (write)
-		swap(src_addr, dst_addr);
-
-	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
-				len, 0);
-
+	while (src_mm.remaining) {
+		src_addr = amdgpu_ttm_domain_start(adev, bo->resource->mem_type) +
+			src_mm.start;
+		dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo) + count;
+		if (write)
+			swap(src_addr, dst_addr);
+
+		amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
+					src_mm.size, 0);
+
+		count += src_mm.size;
+		amdgpu_res_next(&src_mm, src_mm.size);
+    }
 	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
 	WARN_ON(job->ibs[0].length_dw > num_dw);
 
-- 
2.25.1

Re: [PATCH 1/2] drm/amdgpu: fix KFDMemoryTest.PtraceAccessInvisibleVram fail on SRIOV

[Felix Kuehling]

On 2024-08-12 02:59, Samuel Zhang wrote:
> Ptrace access VRAM bo will first try sdma access in
> amdgpu_ttm_access_memory_sdma(), if fails, it will fallback to mmio
> access.
>
> Since ptrace only access 8 bytes at a time and
> amdgpu_ttm_access_memory_sdma() only allow PAGE_SIZE bytes access,
> it returns fail.
> On SRIOV, mmio access will also fail as MM_INDEX/MM_DATA register write
> is blocked for security reasons.
>
> The fix is just change len check in amdgpu_ttm_access_memory_sdma() so
> that len in (0, PAGE_SIZE] are allowed. This will not only fix the ptrace
> test case on SRIOV, but also improve the access performance when the
> access length is < PAGE_SIZE.
> len > PAGE_SIZE case support is not needed as larger size will be break
> into chunks of PAGE_SIZE len max in mem_rw().
>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 5daa05e23ddf..a6e90eada367 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1486,7 +1486,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	unsigned int num_dw;
>   	int r, idx;
>   
> -	if (len != PAGE_SIZE)
> +	if (len > PAGE_SIZE)
OK, I'll spell it out explicitly. This needs an SRIOV VF-specific 
condition if you want to allow smaller accesses with SDMA on SRIOV. On 
bare metal we want to be able to fall back to the FB BAR for smaller 
accesses. On a VF it will use SDMA for everything.

	if (!amdgpu_sriov_vf(adev) && len != PAGE_SIZE)
		return -EINVAL;


Regards,
   Felix


>   		return -EINVAL;
>   
>   	if (!adev->mman.sdma_access_ptr)
> @@ -1514,7 +1514,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   		swap(src_addr, dst_addr);
>   
>   	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
> -				PAGE_SIZE, 0);
> +				len, 0);
>   
>   	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
>   	WARN_ON(job->ibs[0].length_dw > num_dw);

Re: [PATCH 2/2] drm/amdgpu: fix incomplete access issue in amdgpu_ttm_access_memory_sdma()

[Felix Kuehling]

On 2024-08-12 02:59, Samuel Zhang wrote:
> The requested access range may be across 2 adjacent buddy blocks of a
> BO. In this case, it needs to issue 2 sdma copy commands to fully access
> the data range. But current implementation only issue 1 sdma copy
> command and result in incomplete access.
>
> The fix is to loop the res cursor when emitting copy commands so that
> multiple(2) copy commands got issued when necessary.
>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 26 ++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index a6e90eada367..c423574acd5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1484,7 +1484,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	struct dma_fence *fence;
>   	uint64_t src_addr, dst_addr;
>   	unsigned int num_dw;
> -	int r, idx;
> +	int r, idx, count = 0;
>   
>   	if (len > PAGE_SIZE)
>   		return -EINVAL;
> @@ -1498,7 +1498,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	if (write)
>   		memcpy(adev->mman.sdma_access_ptr, buf, len);
>   
> -	num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw, 8);
> +	num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw * 2, 8);
>   	r = amdgpu_job_alloc_with_ib(adev, &adev->mman.high_pr,
>   				     AMDGPU_FENCE_OWNER_UNDEFINED,
>   				     num_dw * 4, AMDGPU_IB_POOL_DELAYED,
> @@ -1507,15 +1507,19 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   		goto out;
>   
>   	amdgpu_res_first(abo->tbo.resource, offset, len, &src_mm);
> -	src_addr = amdgpu_ttm_domain_start(adev, bo->resource->mem_type) +
> -		src_mm.start;
> -	dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo);
> -	if (write)
> -		swap(src_addr, dst_addr);
> -
> -	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
> -				len, 0);
> -
> +	while (src_mm.remaining) {
> +		src_addr = amdgpu_ttm_domain_start(adev, bo->resource->mem_type) +
> +			src_mm.start;
> +		dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo) + count;
> +		if (write)
> +			swap(src_addr, dst_addr);
> +
> +		amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
> +					src_mm.size, 0);
> +
> +		count += src_mm.size;

You could just increment dst_addr instead. And move the initialization 
of dst_addr outside the loop. Other than that, this patch is

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> +		amdgpu_res_next(&src_mm, src_mm.size);
> +    }
>   	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
>   	WARN_ON(job->ibs[0].length_dw > num_dw);
>   

Re: [PATCH 1/2] drm/amdgpu: fix KFDMemoryTest.PtraceAccessInvisibleVram fail on SRIOV

[Christian König]

Am 12.08.24 um 08:59 schrieb Samuel Zhang:
> Ptrace access VRAM bo will first try sdma access in
> amdgpu_ttm_access_memory_sdma(), if fails, it will fallback to mmio
> access.
>
> Since ptrace only access 8 bytes at a time and
> amdgpu_ttm_access_memory_sdma() only allow PAGE_SIZE bytes access,
> it returns fail.
> On SRIOV, mmio access will also fail as MM_INDEX/MM_DATA register write
> is blocked for security reasons.
>
> The fix is just change len check in amdgpu_ttm_access_memory_sdma() so
> that len in (0, PAGE_SIZE] are allowed. This will not only fix the ptrace
> test case on SRIOV, but also improve the access performance when the
> access length is < PAGE_SIZE.
> len > PAGE_SIZE case support is not needed as larger size will be break
> into chunks of PAGE_SIZE len max in mem_rw().
>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 5daa05e23ddf..a6e90eada367 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1486,7 +1486,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>  	unsigned int num_dw;
>  	int r, idx;
>  
> -	if (len != PAGE_SIZE)
> +	if (len > PAGE_SIZE)
>  		return -EINVAL;

We intentionally avoided that since the MM approach is usually preferable as long as you don't transmit large amounts of data.

You could only add a check here for SRIOV.

Regards,
Christian.

>  
>  	if (!adev->mman.sdma_access_ptr)
> @@ -1514,7 +1514,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>  		swap(src_addr, dst_addr);
>  
>  	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr,
> -				PAGE_SIZE, 0);
> +				len, 0);
>  
>  	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
>  	WARN_ON(job->ibs[0].length_dw > num_dw);