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

[Francois Dugast <francois.dugast@intel.com>]

On Wed, Nov 29, 2023 at 06:02:54PM +0100, Souza, Jose wrote:
> On Wed, 2023-11-22 at 14:38 +0000, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > 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.
> 
> Now that the PAT uAPI landed this patch needs alignment fixes in drm_xe_gem_create.

I will fix it during rebase, thanks.

Francois

> 
> > 
> > v2: Fix remaining wholes and double check with pahole (Jose)
> >     Ensure with pahole that both 32b and 64b have exact same
> >     layout (Thomas)
> >     Do not set query's pad and reserved bits to zero since it
> >     is redundant and already done by kzalloc (Matt)
> > 
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_query.c |  1 +
> >  drivers/gpu/drm/xe/xe_vm.c    |  8 ++++++++
> >  include/uapi/drm/xe_drm.h     | 19 +++++++++++--------
> >  3 files changed, 20 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > index 095f1b8c2290..d0523dfc488f 100644
> > --- a/drivers/gpu/drm/xe/xe_query.c
> > +++ b/drivers/gpu/drm/xe/xe_query.c
> > @@ -371,6 +371,7 @@ 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;
> > +
> >  	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;
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index ccd2821e61f8..1dbd6e948741 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2817,6 +2817,10 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
> >  	int err;
> >  	int i;
> >  
> > +	if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> > +	    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))
> > @@ -2933,6 +2937,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 || args->pad2) ||
> > +	    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 b0ef1f62fd99..4c175946547c 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -211,8 +211,6 @@ struct drm_xe_mem_region {
> >  	 * a unique pair.
> >  	 */
> >  	__u16 instance;
> > -	/** @pad: MBZ */
> > -	__u32 pad;
> >  	/**
> >  	 * @min_page_size: Min page-size in bytes for this region.
> >  	 *
> > @@ -381,6 +379,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];
> >  	/** @reference_clock: A clock frequency for timestamp */
> >  	__u32 reference_clock;
> >  	/**
> > @@ -715,6 +715,9 @@ struct drm_xe_vm_bind_op {
> >  	 */
> >  	__u32 prefetch_mem_region_instance;
> >  
> > +	/** @pad: MBZ */
> > +	__u32 pad2;
> > +
> >  	/** @reserved: Reserved */
> >  	__u64 reserved[3];
> >  };
> > @@ -733,12 +736,12 @@ struct drm_xe_vm_bind {
> >  	 */
> >  	__u32 exec_queue_id;
> >  
> > -	/** @num_binds: number of binds in this IOCTL */
> > -	__u32 num_binds;
> > -
> >  	/** @pad: MBZ */
> >  	__u32 pad;
> >  
> > +	/** @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;
> > @@ -750,12 +753,12 @@ struct drm_xe_vm_bind {
> >  		__u64 vector_of_binds;
> >  	};
> >  
> > +	/** @pad: MBZ */
> > +	__u32 pad2;
> > +
> >  	/** @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;
> >  
>