Re: [RFC 7/7] drm/xe/uapi: Wire up GEM_CREATE NUMA node placement handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe: UAPI mismatch for VRAM-only BOs in gem_create_parse_numa_node()
- [Low] drm/xe: missing numa_node kernel-doc in xe_bo_init_locked()
--

commit 4dfdcc93fc9d778a45958834231a36c288d379fc
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

drm/xe/uapi: Wire up GEM_CREATE NUMA node placement handler

This commit parses the DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE user
extension prior to BO creation in xe_gem_create_ioctl() and passes the
requested NUMA node ID down through the BO creation helpers to store on
the xe_bo.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index fc9bfb436b8e7..bbbc337372641 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c

[ ... ]

> @@ -2480,7 +2480,7 @@ struct xe_bo *xe_bo_init_locked(struct xe_device *xe, struct xe_bo *bo,
>  				struct ttm_lru_bulk_move *bulk, size_t size,
>  				u16 cpu_caching, enum ttm_bo_type type,
>  				u32 flags, struct dma_buf *dma_buf,
> -				struct drm_exec *exec)
> +				int numa_node, struct drm_exec *exec)
>  {

[ ... ]

> @@ -2966,7 +2967,8 @@ static void xe_bo_debug_mixed_mode_cur_index_advance(struct xe_device *xe,
>  struct xe_bo *xe_bo_create_user(struct xe_device *xe,
>  				struct xe_vm *vm, size_t size,
>  				u16 cpu_caching,
> -				u32 flags, struct drm_exec *exec)
> +				u32 flags, int numa_node,
> +				struct drm_exec *exec)
>  {

[Severity: Low]
This isn't a bug, but both xe_bo_init_locked() and xe_bo_create_user() have
kernel-doc comment blocks immediately above their definitions. Should
@numa_node be added to both kernel-doc blocks to avoid W=1 kernel-doc
warnings for undescribed function parameters?

[ ... ]

> @@ -3658,6 +3666,56 @@ static int gem_create_user_extensions(struct xe_device *xe, struct xe_bo *bo,
>  	return 0;
>  }
>  
> +/*
> + * Parse a host NUMA node request from the extension chain. The node must be
> + * known before the BO is created, because the backing pages are allocated
> + * during creation. Returns the requested node in @numa_node (NUMA_NO_NODE if
> + * none or a negative value is given) or a negative errno on an invalid request.
> + */
> +static int gem_create_parse_numa_node(struct xe_device *xe, u64 extensions,
> +				      u32 bo_flags, int *numa_node)
> +{
> +	struct drm_xe_user_extension base;
> +	struct drm_xe_ext_set_property ext;
> +	unsigned int i = 0;
> +
> +	*numa_node = NUMA_NO_NODE;
> +
> +	while (extensions && i++ < MAX_USER_EXTENSIONS) {
> +		u64 __user *address = u64_to_user_ptr(extensions);
> +		s64 nid;
> +
> +		if (XE_IOCTL_DBG(xe, copy_from_user(&base, address, sizeof(base))))
> +			return -EFAULT;
> +
> +		if (base.name != DRM_XE_GEM_CREATE_EXTENSION_SET_PROPERTY)
> +			goto next;
> +
> +		if (XE_IOCTL_DBG(xe, copy_from_user(&ext, address, sizeof(ext))))
> +			return -EFAULT;
> +
> +		if (ext.property != DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE)
> +			goto next;
> +
> +		nid = (s64)ext.value;
> +		*numa_node = NUMA_NO_NODE;
> +		if (nid < 0)
> +			goto next;
> +
> +		if (XE_IOCTL_DBG(xe, nid >= MAX_NUMNODES || !node_online(nid)))
> +			return -EINVAL;
> +
> +		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_SYSTEM)))
> +			return -EINVAL;
> +
> +		*numa_node = nid;
> +next:
> +		extensions = base.next_extension;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does rejecting !(bo_flags & XE_BO_FLAG_SYSTEM) with -EINVAL in
xe_gem_create_ioctl() -> gem_create_parse_numa_node() match the UAPI
contract in include/uapi/drm/xe_drm.h?

In struct drm_xe_gem_create, the comment for
DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE states:

	/*
	 * Host NUMA node (as a signed value) to allocate system-memory
	 * backing pages from. Only honored for objects with a system-memory
	 * placement; ignored for VRAM-only objects. Use a value < 0 to keep
	 * the default device-local placement.
	 */
#define   DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE	1

However, gem_create_parse_numa_node() returns -EINVAL when nid >= 0 and
!(bo_flags & XE_BO_FLAG_SYSTEM) instead of ignoring the property on
VRAM-only objects.

In addition, because nid < 0 jumps to next before reaching the
!(bo_flags & XE_BO_FLAG_SYSTEM) check, a VRAM-only BO with nid < 0 is
accepted while nid >= 0 fails with -EINVAL, and an extension chain on a
VRAM-only BO that sets nid >= 0 followed by a later extension resetting
nid = -1 will fail on the first extension.

Should the UAPI comment and gem_create_parse_numa_node() be aligned on
whether VRAM-only BOs ignore DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE or
reject it with -EINVAL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102101803.455921-9-himal.prasad.ghimiray@intel.com?part=7