[Intel-xe] [PATCH 1/2] drm/xe: Add explicit padding to uAPI definition
Lucas De Marchi
lucas.demarchi at intel.com
Wed May 24 16:21:53 UTC 2023
On Tue, May 23, 2023 at 08:31:30PM -0700, Christopher Snowhill wrote:
>Pad the uAPI definition so that it would align identically between
>64-bit and 32-bit uarch, so consumers using this header will work
>correctly from 32-bit compat userspace on a 64-bit kernel. Do it
>in a minimally invasive way, so that 64-bit userspace will still
>work with the previous header, and so that no fields suddenly
>change sizes.
>
>Originally inspired by mlankhorst.
>
>Signed-off-by: Christopher Snowhill <kode54 at gmail.com>
Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
... with the commands below. As a follow up, should we add a way to
verify? And probably do the reordering in lockstep with userspace
changes ahead of merging upstream and committing to the ABI?
Checking:
$ sed -n 's/^struct \(.*\) {/struct \1 \1;/p' include/uapi/drm/xe_drm.h > /tmp/a.c
$ gcc -Iinclude/ -I include/uapi/ -include include/uapi/drm/xe_drm.h -g -c -o /tmp/a.o /tmp/a.c
$ pahole /tmp/a.o > /tmp/a.txt
$ # ... apply this patch
$ sed -n 's/^struct \(.*\) {/struct \1 \1;/p' include/uapi/drm/xe_drm.h > /tmp/b.c
$ gcc -Iinclude/ -I include/uapi/ -include include/uapi/drm/xe_drm.h -g -c -o /tmp/b.o /tmp/b.c
$ pahole /tmp/b.o > /tmp/b.txt
$ grep hole /tmp/b.txt
$ # good, no holes
$ # double checking the fixes in this patch
$ git diff --no-index /tmp/a.txt /tmp/b.txt
diff --git a/tmp/a.txt b/tmp/b.txt
index bcc2fd7a97cd..715687ad8c4f 100644
--- a/tmp/a.txt
+++ b/tmp/b.txt
@@ -89,13 +89,10 @@ struct drm_xe_gem_create {
__u32 flags; /* 16 4 */
__u32 vm_id; /* 20 4 */
__u32 handle; /* 24 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 28 4 */
__u64 reserved[2]; /* 32 16 */
- /* size: 48, cachelines: 1, members: 6 */
- /* sum members: 44, holes: 1, sum holes: 4 */
+ /* size: 48, cachelines: 1, members: 7 */
/* last cacheline: 48 bytes */
};
struct drm_xe_gem_mmap_offset {
@@ -120,14 +117,11 @@ struct drm_xe_vm_bind_op_error_capture {
struct drm_xe_ext_vm_set_property {
struct xe_user_extension base; /* 0 16 */
__u32 property; /* 16 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 20 4 */
__u64 value; /* 24 8 */
__u64 reserved[2]; /* 32 16 */
- /* size: 48, cachelines: 1, members: 4 */
- /* sum members: 44, holes: 1, sum holes: 4 */
+ /* size: 48, cachelines: 1, members: 5 */
/* last cacheline: 48 bytes */
};
struct drm_xe_vm_create {
@@ -149,9 +143,7 @@ struct drm_xe_vm_destroy {
};
struct drm_xe_vm_bind_op {
__u32 obj; /* 0 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 4 4 */
union {
__u64 obj_offset; /* 8 8 */
__u64 userptr; /* 8 8 */
@@ -163,43 +155,34 @@ struct drm_xe_vm_bind_op {
__u32 region; /* 44 4 */
__u64 reserved[2]; /* 48 16 */
- /* size: 64, cachelines: 1, members: 8 */
- /* sum members: 60, holes: 1, sum holes: 4 */
+ /* size: 64, cachelines: 1, members: 9 */
};
struct drm_xe_vm_bind {
__u64 extensions; /* 0 8 */
__u32 vm_id; /* 8 4 */
__u32 engine_id; /* 12 4 */
__u32 num_binds; /* 16 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 20 4 */
union {
struct drm_xe_vm_bind_op bind; /* 24 64 */
__u64 vector_of_binds; /* 24 8 */
}; /* 24 64 */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
__u32 num_syncs; /* 88 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad2; /* 92 4 */
__u64 syncs; /* 96 8 */
__u64 reserved[2]; /* 104 16 */
- /* size: 120, cachelines: 2, members: 8 */
- /* sum members: 112, holes: 2, sum holes: 8 */
+ /* size: 120, cachelines: 2, members: 10 */
/* last cacheline: 56 bytes */
};
struct drm_xe_ext_engine_set_property {
struct xe_user_extension base; /* 0 16 */
__u32 property; /* 16 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 20 4 */
__u64 value; /* 24 8 */
- /* size: 32, cachelines: 1, members: 3 */
- /* sum members: 28, holes: 1, sum holes: 4 */
+ /* size: 32, cachelines: 1, members: 4 */
/* last cacheline: 32 bytes */
};
struct drm_xe_engine_set_property {
@@ -246,9 +229,7 @@ struct drm_xe_engine_destroy {
struct drm_xe_sync {
__u64 extensions; /* 0 8 */
__u32 flags; /* 8 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 12 4 */
union {
__u32 handle; /* 16 4 */
__u64 addr; /* 16 8 */
@@ -256,8 +237,7 @@ struct drm_xe_sync {
__u64 timeline_value; /* 24 8 */
__u64 reserved[2]; /* 32 16 */
- /* size: 48, cachelines: 1, members: 5 */
- /* sum members: 44, holes: 1, sum holes: 4 */
+ /* size: 48, cachelines: 1, members: 6 */
/* last cacheline: 48 bytes */
};
struct drm_xe_exec {
@@ -267,13 +247,10 @@ struct drm_xe_exec {
__u64 syncs; /* 16 8 */
__u64 address; /* 24 8 */
__u16 num_batch_buffer; /* 32 2 */
-
- /* XXX 6 bytes hole, try to pack */
-
+ __u16 pad[3]; /* 34 6 */
__u64 reserved[2]; /* 40 16 */
- /* size: 56, cachelines: 1, members: 7 */
- /* sum members: 50, holes: 1, sum holes: 6 */
+ /* size: 56, cachelines: 1, members: 8 */
/* last cacheline: 56 bytes */
};
struct drm_xe_mmio {
@@ -294,9 +271,7 @@ struct drm_xe_wait_user_fence {
}; /* 8 8 */
__u16 op; /* 16 2 */
__u16 flags; /* 18 2 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 20 4 */
__u64 value; /* 24 8 */
__u64 mask; /* 32 8 */
__s64 timeout; /* 40 8 */
@@ -305,25 +280,19 @@ struct drm_xe_wait_user_fence {
/* --- cacheline 1 boundary (64 bytes) --- */
__u64 reserved[2]; /* 64 16 */
- /* size: 80, cachelines: 2, members: 10 */
- /* sum members: 76, holes: 1, sum holes: 4 */
+ /* size: 80, cachelines: 2, members: 11 */
/* last cacheline: 16 bytes */
};
struct drm_xe_vm_madvise {
__u64 extensions; /* 0 8 */
__u32 vm_id; /* 8 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad; /* 12 4 */
__u64 range; /* 16 8 */
__u64 addr; /* 24 8 */
__u32 property; /* 32 4 */
-
- /* XXX 4 bytes hole, try to pack */
-
+ __u32 pad2; /* 36 4 */
__u64 value; /* 40 8 */
__u64 reserved[2]; /* 48 16 */
- /* size: 64, cachelines: 1, members: 7 */
- /* sum members: 56, holes: 2, sum holes: 8 */
+ /* size: 64, cachelines: 1, members: 9 */
};
Lucas De Marchi
>---
> include/uapi/drm/xe_drm.h | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index b0b80aae3ee8..d5fc54b5be74 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -91,7 +91,7 @@ struct xe_user_extension {
> */
> __u32 name;
> /**
>- * @flags: MBZ
>+ * @pad: MBZ
> *
> * All undefined bits must be zero.
> */
>@@ -291,6 +291,9 @@ struct drm_xe_gem_create {
> */
> __u32 handle;
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> /** @reserved: Reserved */
> __u64 reserved[2];
> };
>@@ -335,6 +338,9 @@ struct drm_xe_ext_vm_set_property {
> #define XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS 0
> __u32 property;
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> /** @value: property value */
> __u64 value;
>
>@@ -379,6 +385,9 @@ struct drm_xe_vm_bind_op {
> */
> __u32 obj;
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> union {
> /**
> * @obj_offset: Offset into the object, MBZ for CLEAR_RANGE,
>@@ -469,6 +478,9 @@ 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;
>@@ -482,6 +494,9 @@ 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;
>
>@@ -497,6 +512,9 @@ struct drm_xe_ext_engine_set_property {
> /** @property: property to set */
> __u32 property;
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> /** @value: property value */
> __u64 value;
> };
>@@ -612,6 +630,9 @@ struct drm_xe_sync {
> #define DRM_XE_SYNC_USER_FENCE 0x3
> #define DRM_XE_SYNC_SIGNAL 0x10
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> union {
> __u32 handle;
> /**
>@@ -656,6 +677,9 @@ struct drm_xe_exec {
> */
> __u16 num_batch_buffer;
>
>+ /** @pad: MBZ */
>+ __u16 pad[3];
>+
> /** @reserved: Reserved */
> __u64 reserved[2];
> };
>@@ -718,6 +742,8 @@ struct drm_xe_wait_user_fence {
> #define DRM_XE_UFENCE_WAIT_ABSTIME (1 << 1)
> #define DRM_XE_UFENCE_WAIT_VM_ERROR (1 << 2)
> __u16 flags;
>+ /** @pad: MBZ */
>+ __u32 pad;
> /** @value: compare value */
> __u64 value;
> /** @mask: comparison mask */
>@@ -750,6 +776,9 @@ struct drm_xe_vm_madvise {
> /** @vm_id: The ID VM in which the VMA exists */
> __u32 vm_id;
>
>+ /** @pad: MBZ */
>+ __u32 pad;
>+
> /** @range: Number of bytes in the VMA */
> __u64 range;
>
>@@ -794,6 +823,9 @@ struct drm_xe_vm_madvise {
> /** @property: property to set */
> __u32 property;
>
>+ /** @pad2: MBZ */
>+ __u32 pad2;
>+
> /** @value: property value */
> __u64 value;
>
>--
>2.40.1
>
More information about the Intel-xe
mailing list