[Intel-xe] [PATCH] drm/xe/uapi: Fix various struct padding for 64b alignment

[Matt Roper]

On Thu, Nov 16, 2023 at 11:06:25PM -0500, Rodrigo Vivi wrote:
> Let's respect Documentation/process/botching-up-ioctls.rst
> and add the proper padding for a 64b alignment with all as
> well as all the required checks and settings for the pads
> and the reserved entries.
> 
> Cc: Francois Dugast <francois.dugast at intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_query.c |  6 ++++++
>  drivers/gpu/drm/xe/xe_vm.c    |  7 +++++++
>  include/uapi/drm/xe_drm.h     | 14 +++++---------
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 838f03795841..77c3afeaf4b6 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -277,6 +277,7 @@ static int query_mem_region(struct xe_device *xe,
>  	if (perfmon_capable())
>  		usage->mem_regions[0].used = ttm_resource_manager_usage(man);
>  	usage->num_mem_regions = 1;
> +	usage->pad = 0;

Why are we explicitly setting all these pads?  The important part was
adding these pad fields to the structure definition so that both 32-bit
and 64-bit would consider the structure to be the same 64-bit-aligned
size.  The kzalloc should always zero the entire structure correctly
now, making these additional pad clears redundant.


Matt

>  
>  	for (i = XE_PL_VRAM0; i <= XE_PL_VRAM1; ++i) {
>  		man = ttm_manager_type(&xe->ttm, i);
> @@ -335,6 +336,7 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
>  		return -ENOMEM;
>  
>  	config->num_params = num_params;
> +	config->pad = 0;
>  	config->info[DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID] =
>  		xe->info.devid | (xe->info.revid << 16);
>  	if (xe_device_get_root_tile(xe)->mem.vram.usable_size)
> @@ -377,6 +379,8 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query
>  		return -ENOMEM;
>  
>  	gt_list->num_gt = xe->info.gt_count;
> +	gt_list->pad = 0;
> +
>  	for_each_gt(gt, xe, id) {
>  		if (xe_gt_is_media_type(gt))
>  			gt_list->gt_list[id].type = DRM_XE_QUERY_GT_TYPE_MEDIA;
> @@ -392,6 +396,8 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query
>  				BIT(gt_to_tile(gt)->id) << 1;
>  		gt_list->gt_list[id].far_mem_regions = xe->info.mem_region_mask ^
>  			gt_list->gt_list[id].near_mem_regions;
> +		memset(gt_list->gt_list[id].reserved, 0,
> +		       sizeof(gt_list->gt_list[id].reserved));
>  	}
>  
>  	if (copy_to_user(query_ptr, gt_list, size)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f8559ebad9bc..6e09855f1e2f 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2850,6 +2850,9 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>  	int err;
>  	int i;
>  
> +	if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> +		return -EINVAL;
> +
>  	if (XE_IOCTL_DBG(xe, args->extensions) ||
>  	    XE_IOCTL_DBG(xe, !args->num_binds) ||
>  	    XE_IOCTL_DBG(xe, args->num_binds > MAX_BINDS))
> @@ -2966,6 +2969,10 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  	if (err)
>  		return err;
>  
> +	if (XE_IOCTL_DBG(xe, args->pad) ||
> +	    XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> +		return -EINVAL;
> +
>  	if (args->exec_queue_id) {
>  		q = xe_exec_queue_lookup(xef, args->exec_queue_id);
>  		if (XE_IOCTL_DBG(xe, !q)) {
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 8610ac461619..777d2564bcdf 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -384,6 +384,8 @@ struct drm_xe_gt {
>  	__u16 tile_id;
>  	/** @gt_id: Unique ID of this GT within the PCI Device */
>  	__u16 gt_id;
> +	/** @pad: MBZ */
> +	__u16 pad[3];
>  	/** @clock_freq: A clock frequency for timestamp */
>  	__u32 clock_freq;
>  	/**
> @@ -662,9 +664,6 @@ struct drm_xe_vm_bind_op {
>  	 */
>  	__u32 obj;
>  
> -	/** @pad: MBZ */
> -	__u32 pad;
> -
>  	union {
>  		/**
>  		 * @obj_offset: Offset into the object, MBZ for CLEAR_RANGE,
> @@ -744,9 +743,6 @@ struct drm_xe_vm_bind {
>  	/** @num_binds: number of binds in this IOCTL */
>  	__u32 num_binds;
>  
> -	/** @pad: MBZ */
> -	__u32 pad;
> -
>  	union {
>  		/** @bind: used if num_binds == 1 */
>  		struct drm_xe_vm_bind_op bind;
> @@ -761,12 +757,12 @@ struct drm_xe_vm_bind {
>  	/** @num_syncs: amount of syncs to wait on */
>  	__u32 num_syncs;
>  
> -	/** @pad2: MBZ */
> -	__u32 pad2;
> -
>  	/** @syncs: pointer to struct drm_xe_sync array */
>  	__u64 syncs;
>  
> +	/** @pad: MBZ */
> +	__u64 pad;
> +
>  	/** @reserved: Reserved */
>  	__u64 reserved[2];
>  };
> -- 
> 2.41.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation