Re: [PATCH 3/7] drm/xe: Add xe_ggtt_node_addr() to avoid dereferencing xe_ggtt_node

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/21/2025 2:18 PM, Maarten Lankhorst wrote:
> This function makes it possible to add an offset that is applied to
> all xe_ggtt_node's, and hides the internals from all its users.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_bo.h   |  8 +++++---
>  drivers/gpu/drm/xe/xe_ggtt.c | 11 +++++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h |  2 ++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index 911d5b90461a2..9c3f0ebaa1554 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -9,6 +9,7 @@
>  #include <drm/ttm/ttm_tt.h>
>  
>  #include "xe_bo_types.h"
> +#include "xe_ggtt.h"
>  #include "xe_macros.h"
>  #include "xe_validation.h"
>  #include "xe_vm_types.h"
> @@ -251,13 +252,14 @@ static inline u32
>  __xe_bo_ggtt_addr(struct xe_bo *bo, u8 tile_id)
>  {
>  	struct xe_ggtt_node *ggtt_node = bo->ggtt_node[tile_id];
> +	u64 offset;
>  
>  	if (XE_WARN_ON(!ggtt_node))
>  		return 0;
>  
> -	XE_WARN_ON(ggtt_node->base.size > xe_bo_size(bo));
> -	XE_WARN_ON(ggtt_node->base.start + ggtt_node->base.size > (1ull << 32));
> -	return ggtt_node->base.start;
> +	offset = xe_ggtt_node_addr(ggtt_node);
> +	XE_WARN_ON(offset + xe_bo_size(bo) > (1ull << 32));

maybe better to use U32_MAX and convert to:

	xe_assert(xe_bo_device(bo), offset + xe_bo_size(bo) <= U32_MAX);

or at least to xe-oriented:

	xe_WARN_ON(xe_bo_device(bo), offset + xe_bo_size(bo) > U32_MAX);

and what about checking bo_size vs node_size ?

> +	return offset;
>  }
>  
>  static inline u32
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 42e3caa3a6d9c..05515037e2d6a 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -1079,3 +1079,14 @@ u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset)
>  {
>  	return ioread64(ggtt->gsm + (offset / XE_PAGE_SIZE));
>  }
> +
> +/**
> + * xe_ggtt_node_addr - Get @node offset in GGTT.
> + * @node: &xe_ggtt_node
> + *
> + * Get the GGTT offset for allocated node.
> + */
> +u64 xe_ggtt_node_addr(const struct xe_ggtt_node *node)
> +{

we can also have here:

	xe_ggtt_assert_fit(node->ggtt, node->base.start, node->base.size);

> +	return node->base.start;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index d54a2ae2b4aa8..4a8ef1b824156 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -58,4 +58,6 @@ void xe_ggtt_might_lock(struct xe_ggtt *ggtt);
>  u64 xe_ggtt_encode_pte_flags(struct xe_ggtt *ggtt, struct xe_bo *bo, u16 pat_index);
>  u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset);
>  
> +u64 xe_ggtt_node_addr(const struct xe_ggtt_node *node);

nit: shouldn't we keep "node" related functions together ?

> +
>  #endif