[Intel-xe] [PATCH] drm/xe/uapi: Fix various struct padding for 64b alignment
Souza, Jose
jose.souza at intel.com
Fri Nov 17 14:25:56 UTC 2023
On Thu, 2023-11-16 at 23:06 -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;
>
> 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];
> };
Found more issues, with this LGTM:
diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h
index 39da4b08182..2eb6ef225a8 100644
--- a/include/drm-uapi/xe_drm.h
+++ b/include/drm-uapi/xe_drm.h
@@ -177,8 +177,6 @@ struct drm_xe_query_mem_region {
* a unique pair.
*/
__u16 instance;
- /** @pad: MBZ */
- __u32 pad;
/**
* @min_page_size: Min page-size in bytes for this region.
*
@@ -347,9 +345,11 @@ struct drm_xe_query_gt {
/** @gt_id: Unique ID of this GT within the PCI Device */
__u16 gt_id;
/** @pad: MBZ */
- __u16 pad[3];
+ __u16 pad[2];
/** @clock_freq: A clock frequency for timestamp */
__u32 clock_freq;
+ __u32 pad2;
+
/**
* @near_mem_regions: Bit mask of instances from
* drm_xe_query_mem_regions that is near the current engines of this GT.
@@ -620,6 +620,8 @@ struct drm_xe_vm_bind_op {
*/
__u32 obj;
+ __u32 pad;
+
union {
/**
* @obj_offset: Offset into the object, MBZ for CLEAR_RANGE,
@@ -678,6 +680,8 @@ struct drm_xe_vm_bind_op {
*/
__u32 prefetch_mem_region_instance;
+ __u32 pad2;
+
/** @reserved: Reserved */
__u64 reserved[2];
};
@@ -699,20 +703,20 @@ struct drm_xe_vm_bind {
/** @num_binds: number of binds in this IOCTL */
__u32 num_binds;
- union {
- /** @bind: used if num_binds == 1 */
- struct drm_xe_vm_bind_op bind;
-
- /**
- * @vector_of_binds: userptr to array of struct
- * drm_xe_vm_bind_op if num_binds > 1
- */
- __u64 vector_of_binds;
- };
-
/** @num_syncs: amount of syncs to wait on */
__u32 num_syncs;
+ union {
+ /** @bind: used if num_binds == 1 */
+ struct drm_xe_vm_bind_op bind;
+
+ /**
+ * @vector_of_binds: userptr to array of struct
+ * drm_xe_vm_bind_op if num_binds > 1
+ */
+ __u64 vector_of_binds;
+ };
+
/** @syncs: pointer to struct drm_xe_sync array */
__u64 syncs;
You can verify it with pahole, maybe we could have that running in CI so we don't break it anymore.
More information about the Intel-xe
mailing list