Re: [PATCH v9 01/18] drm/amdgpu: add SVM ioctl UAPI definitions

["Christian König" <christian.koenig@amd.com>]

On 8/4/26 11:42, Huang Rui wrote:
> From: Honglei Huang <honghuan@amd.com>
> 
> Define the following ioctl structures and enums:
> - DRM_AMDGPU_GEM_SVM ioctl command and DRM_IOCTL_AMDGPU_GEM_SVM macro
> - enum amdgpu_ioctl_svm_op: SET_ATTR, GET_ATTR, RESET_ATTR operations
> - enum amdgpu_ioctl_svm_access: INACCESSIBLE, IN_PLACE, ALLOW_MIGRATE
> - enum amdgpu_ioctl_svm_location: SYSMEM, UNDEFINED
> - enum amdgpu_ioctl_svm_attr_type: PREFERRED_LOC, PREFETCH_LOC, ACCESS,
>   GRANULARITY, HOST_ACCESS, COHERENT, EXT_COHERENT, HIVE_LOCAL, GPU_RO,
>   GPU_EXEC, GPU_READ_MOSTLY
> - struct drm_amdgpu_svm_attribute: type and value pair
> - struct drm_amdgpu_gem_svm: ioctl payload with start_addr, size,
>   operation, nattr, and attrs_ptr
> 
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  include/uapi/drm/amdgpu_drm.h | 106 ++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index b32c72a662b61..4c49cd36f0e77 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -59,6 +59,7 @@ extern "C" {
>  #define DRM_AMDGPU_USERQ_WAIT		0x18
>  #define DRM_AMDGPU_GEM_LIST_HANDLES	0x19
>  #define DRM_AMDGPU_PROC_OPTIONS		0x1A
> +#define DRM_AMDGPU_GEM_SVM		0x1B
>  
>  #define DRM_IOCTL_AMDGPU_GEM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_CREATE, union drm_amdgpu_gem_create)
>  #define DRM_IOCTL_AMDGPU_GEM_MMAP	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_MMAP, union drm_amdgpu_gem_mmap)
> @@ -81,6 +82,7 @@ extern "C" {
>  #define DRM_IOCTL_AMDGPU_USERQ_WAIT	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_USERQ_WAIT, struct drm_amdgpu_userq_wait)
>  #define DRM_IOCTL_AMDGPU_GEM_LIST_HANDLES DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_LIST_HANDLES, struct drm_amdgpu_gem_list_handles)
>  #define DRM_IOCTL_AMDGPU_PROC_OPTIONS	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_PROC_OPTIONS, struct drm_amdgpu_proc_options)
> +#define DRM_IOCTL_AMDGPU_GEM_SVM	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_SVM, struct drm_amdgpu_gem_svm)
>  
>  /**
>   * DOC: memory domains
> @@ -1694,6 +1696,110 @@ struct drm_amdgpu_proc_options {
>  	} kfd_sigbus_delay;
>  };
>  
> +/**
> + * enum amdgpu_ioctl_svm_op - operation selector for DRM_IOCTL_AMDGPU_GEM_SVM.
> + * @AMDGPU_SVM_OP_SET_ATTR: apply the attributes in @attrs_ptr to the VA range.
> + * @AMDGPU_SVM_OP_GET_ATTR: read back the current value of each attribute
> + *                          listed in @attrs_ptr for the given VA range.

That this works on the given VA range is not correct.

To read out the attributes userspace needs to give the start addr and attributes and get the size the attributes are the same in return.

Only this way CRIU is able to read out the attributes from kernel to userspace.

> + * @AMDGPU_SVM_OP_RESET_ATTR: reset all attributes for the VA range to their
> + *                            default values. @attrs_ptr and @nattr are ignored.
> + */
> +enum amdgpu_ioctl_svm_op {
> +	AMDGPU_SVM_OP_SET_ATTR = 0,
> +	AMDGPU_SVM_OP_GET_ATTR = 1,
> +	AMDGPU_SVM_OP_RESET_ATTR = 2,
> +};
> +
> +/**
> + * enum amdgpu_ioctl_svm_access - values for AMDGPU_SVM_ATTR_ACCESS.
> + * @AMDGPU_SVM_ACCESS_INACCESSIBLE: GPU must not access the range; any access
> + *                                  is a fault.
> + * @AMDGPU_SVM_ACCESS_IN_PLACE:     GPU may access the range
This needs more.  only at its
> + *                                  current backing store; the driver will
> + *                                  never migrate pages to local VRAM.
> + * @AMDGPU_SVM_ACCESS_ALLOW_MIGRATE: GPU may access the range and the driver
> + *                                  is allowed (but not required) to migrate
> + *                                  pages between system memory and local
> + *                                  VRAM to satisfy the preferred/prefetch
> + *                                  location.
> + */
> +enum amdgpu_ioctl_svm_access {
> +	AMDGPU_SVM_ACCESS_INACCESSIBLE		= 0,
> +	AMDGPU_SVM_ACCESS_IN_PLACE			= 1,
> +	AMDGPU_SVM_ACCESS_ALLOW_MIGRATE		= 2,
> +};
> +
> +/**
> + * enum amdgpu_svm_location - values for AMDGPU_SVM_ATTR_PREFERRED_LOC /
> + *                            AMDGPU_SVM_ATTR_PREFETCH_LOC.
> + * @AMDGPU_SVM_LOCATION_SYSMEM:    back the range with system memory.
> + * @AMDGPU_SVM_LOCATION_UNDEFINED: no preference; the driver chooses.
> + */
> +enum amdgpu_ioctl_svm_location {
> +	AMDGPU_SVM_LOCATION_SYSMEM	= 0,
> +	AMDGPU_SVM_LOCATION_UNDEFINED	= 0xffffffffU,

Please make the AMDGPU_SVM_LOCATION_UNDEFINED 1 here and add a value AMDGPU_SVM_LOCATION_LOCAL and AMDGPU_SVM_LOCATION_HIVE.

The XE idea of giving the file descriptor of the device driver to prefetch to is a clear NAK from my side now. This whole concept breaks as soon as you add CRIU to the picture.

> +};
> +
> +/**
> + * enum amdgpu_ioctl_svm_attr_type - attribute selector for
> + *                                   &drm_amdgpu_svm_attribute.type.
> + *
> + * @AMDGPU_SVM_ATTR_PREFERRED_LOC: Preferred backing location for the range.
> + *	Value is one of &enum amdgpu_ioctl_svm_location.
> + * @AMDGPU_SVM_ATTR_PREFETCH_LOC: Prefetch target for the range. Value is
> + *	one of &enum amdgpu_ioctl_svm_location.
> + * @AMDGPU_SVM_ATTR_ACCESS: GPU access policy for the range. Value is one
> + *	of &enum amdgpu_ioctl_svm_access.
> + * @AMDGPU_SVM_ATTR_GRANULARITY: log2 of the migration granularity in pages.
> + * @AMDGPU_SVM_ATTR_HOST_ACCESS: Guarantee host access to memory.
> + * @AMDGPU_SVM_ATTR_COHERENT: Fine-grained coherency between all devices
> + *	with access.
> + * @AMDGPU_SVM_ATTR_EXT_COHERENT: Fine-grained coherency between all devices
> + *	using device-scope atomics.

> + * @AMDGPU_SVM_ATTR_HIVE_LOCAL: Use any GPU in the same XGMI hive as the
> + *	preferred device.

Please drop that one, it should be covered by AMDGPU_SVM_LOCATION_* above.

Regards,
Christian.

> + * @AMDGPU_SVM_ATTR_GPU_RO: GPUs only read the range, allowing replication.
> + * @AMDGPU_SVM_ATTR_GPU_EXEC: Allow execution on GPU.
> + * @AMDGPU_SVM_ATTR_GPU_READ_MOSTLY: GPUs mostly read the range; may allow
> + *	optimizations similar to GPU_RO, but writes still fault.
> + */
> +enum amdgpu_ioctl_svm_attr_type {
> +	AMDGPU_SVM_ATTR_PREFERRED_LOC		= 0,
> +	AMDGPU_SVM_ATTR_PREFETCH_LOC		= 1,
> +	AMDGPU_SVM_ATTR_ACCESS				= 2,
> +	AMDGPU_SVM_ATTR_GRANULARITY			= 3,
> +	/* Boolean attributes below: value must be 0 or 1. */
> +	AMDGPU_SVM_ATTR_HOST_ACCESS			= 4,
> +	AMDGPU_SVM_ATTR_COHERENT			= 5,
> +	AMDGPU_SVM_ATTR_EXT_COHERENT		= 6,
> +	AMDGPU_SVM_ATTR_HIVE_LOCAL			= 7,
> +	AMDGPU_SVM_ATTR_GPU_RO				= 8,
> +	AMDGPU_SVM_ATTR_GPU_EXEC			= 9,
> +	AMDGPU_SVM_ATTR_GPU_READ_MOSTLY		= 10,
> +};
> +
> +/* One (type, value) pair carried by DRM_IOCTL_AMDGPU_GEM_SVM. */
> +struct drm_amdgpu_svm_attribute {
> +	/** AMDGPU_SVM_ATTR_* */
> +	__u32 type;
> +	/** Attribute value; interpretation depends on @type */
> +	__u32 value;
> +};
> +
> +/* Argument for DRM_IOCTL_AMDGPU_GEM_SVM. */
> +struct drm_amdgpu_gem_svm {
> +	/** Start of the virtual address range */
> +	__u64 start_addr;
> +	/** Size of the range in bytes */
> +	__u64 size;
> +	/** AMDGPU_SVM_OP_* */
> +	__u32 operation;
> +	/** Number of struct drm_amdgpu_svm_attribute entries in @attrs_ptr */
> +	__u32 nattr;
> +	/** User pointer to an array of @nattr struct drm_amdgpu_svm_attribute */
> +	__u64 attrs_ptr;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif