Re: [PATCH 01/20] drm/ttm: Add ttm_bo_access_locked()

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

Hi Mika,
the description of the "ttm_bo_access()" API states, "Should not be 
directly exported to user space via a peak/poke interface."

When the newly added "ttm_bo_access_locked()" is used in "eudebug", it 
is ultimately exposed to user space via functions such as "pread" and 
"pwrite". @Mattew Brost,I would like to inquire whether this violates 
the considerations made when the "ttm_bo_access()" API was initially 
designed.


Aside from the above point, the added "ttm_bo_access_locked()" API 
appears to be a simple refactoring, so this patch itself does not seem 
to pose any issues.

Br,
G.G.

On 11/9/26 5:55 AM, Mika Kuoppala wrote:
> ttm_bo_access() reserves the buffer object itself, so a caller that
> already holds the reservation cannot use it. Split the body out as
> ttm_bo_access_locked(), which asserts the reservation is held, and leave
> ttm_bo_access() as reserve, access, unreserve.
> 
> The first user, eudebug vm access, has to wait for the move fences and
> access the object under one reservation.
> 
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Michel Dänzer <michel.daenzer@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo_vm.c | 61 +++++++++++++++++++++++----------
>   include/drm/ttm/ttm_bo.h        |  2 ++
>   2 files changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 872bf444b1f0..9a252bce9091 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -405,6 +405,47 @@ static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo,
>   	return len;
>   }
>   
> +/**
> + * ttm_bo_access_locked - Helper to access a reserved buffer object
> + *
> + * @bo: ttm buffer object, reserved by the caller
> + * @offset: access offset into buffer object
> + * @buf: pointer to caller memory to read into or write from
> + * @len: length of access
> + * @write: write access
> + *
> + * Same as ttm_bo_access(), for a caller that already holds the reservation,
> + * for instance to wait for fences before the access without the placement
> + * changing in between.
> + *
> + * Returns:
> + * @len if successful, negative error code on failure.
> + */
> +int ttm_bo_access_locked(struct ttm_buffer_object *bo, unsigned long offset,
> +			 void *buf, int len, int write)
> +{
> +	dma_resv_assert_held(bo->base.resv);
> +
> +	if (len < 1 || (offset + len) > bo->base.size)
> +		return -EIO;
> +
> +	if (!bo->resource)
> +		return -ENODATA;
> +
> +	switch (bo->resource->mem_type) {
> +	case TTM_PL_SYSTEM:
> +		fallthrough;
> +	case TTM_PL_TT:
> +		return ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
> +	default:
> +		if (bo->bdev->funcs->access_memory)
> +			return bo->bdev->funcs->access_memory
> +				(bo, offset, buf, len, write);
> +		return -EIO;
> +	}
> +}
> +EXPORT_SYMBOL(ttm_bo_access_locked);
> +
>   /**
>    * ttm_bo_access - Helper to access a buffer object
>    *
> @@ -433,26 +474,8 @@ int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long offset,
>   	if (ret)
>   		return ret;
>   
> -	if (!bo->resource) {
> -		ret = -ENODATA;
> -		goto unlock;
> -	}
> -
> -	switch (bo->resource->mem_type) {
> -	case TTM_PL_SYSTEM:
> -		fallthrough;
> -	case TTM_PL_TT:
> -		ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write);
> -		break;
> -	default:
> -		if (bo->bdev->funcs->access_memory)
> -			ret = bo->bdev->funcs->access_memory
> -				(bo, offset, buf, len, write);
> -		else
> -			ret = -EIO;
> -	}
> +	ret = ttm_bo_access_locked(bo, offset, buf, len, write);
>   
> -unlock:
>   	ttm_bo_unreserve(bo);
>   
>   	return ret;
> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> index 32791c4db2a9..3b08baa33ffd 100644
> --- a/include/drm/ttm/ttm_bo.h
> +++ b/include/drm/ttm/ttm_bo.h
> @@ -441,6 +441,8 @@ s64 ttm_bo_evict_cgroup(struct ttm_device *bdev,
>   			struct dmem_cgroup_pool_state *limit_pool,
>   			s64 target_bytes,
>   			struct ttm_operation_ctx *ctx);
> +int ttm_bo_access_locked(struct ttm_buffer_object *bo, unsigned long offset,
> +			 void *buf, int len, int write);
>   int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long offset,
>   		  void *buf, int len, int write);
>   vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,