[PATCH v3 0/2] drm/panthor: Fix panthor+FEX-Emu

[PATCH v3 0/2] drm/panthor: Fix panthor+FEX-Emu

[Boris Brezillon]

Hello,

This is an attempt a couple bugs exposed by FEX-Emu. The first one
is pretty trivial and should be uncontroversial, since it's just
a missing padding field in one of our uAPI structs. We are getting
away with it on arm32 because of the alignment rules provided by
the Arm ABI, but x86 has relaxed constraints for u64 fields, and
this bug is definitely hit when running a 32-bit x86 mesa binary
under FEX Emu.

The second fix is addressing a problem we have because FEX-Emu is
an aarch64 process executing 32-bit x86 code, meaning the check
we do on the is-32bit-task check we do to figure out the MMIO
offset seen by the user won't work. In order to fix that, we add
an ioctl to let the user explicitly set this offset. The offset
can only be set early on, if no MMIO range has been mapped before.

With those, and the mesa MR at [1], I managed to run a 32-bit x86
glmark2 through FEX without using the host mesa (if we were to use
the thunked mesa lib, both the kernel and mesa would use
MMIO_OFFSET_64BIT, and the problem doesn't exist anymore).

Regards,

Boris

Changes in v2:
- Simplify the logic in patch2 to have a lockless solution that's
  still safe for what we need

Changes in v3:
- Fix a comment

[1]https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34573

Boris Brezillon (2):
  drm/panthor: Add missing explicit padding in drm_panthor_gpu_info
  drm/panthor: Fix the user MMIO offset logic for emulators

 drivers/gpu/drm/panthor/panthor_device.h | 18 ++++++++
 drivers/gpu/drm/panthor/panthor_drv.c    | 56 +++++++++++++++++-------
 include/uapi/drm/panthor_drm.h           | 41 +++++++++++++++++
 3 files changed, 99 insertions(+), 16 deletions(-)

-- 
2.49.0

[PATCH v3 1/2] drm/panthor: Add missing explicit padding in drm_panthor_gpu_info

[Boris Brezillon]

drm_panthor_gpu_info::shader_present is currently automatically offset
by 4 byte to meet Arm's 32-bit/64-bit field alignment rules, but those
constraints don't stand on 32-bit x86 and cause a mismatch when running
an x86 binary in a user emulated environment like FEX. It's also
generally agreed that uAPIs should explicitly pad their struct fields,
which we originally intended to do, but a mistake slipped through during
the submission process, leading drm_panthor_gpu_info::shader_present to
be misaligned.

This uAPI change doesn't break any of the existing users of panthor
which are either arm32 or arm64 where the 64-bit alignment of
u64 fields is already enforced a the compiler level.

Changes in v2:
- Rename the garbage field into pad0 and adjust the comment accordingly
- Add Liviu's A-b

Changes in v3:
- Add R-bs

Fixes: 0f25e493a246 ("drm/panthor: Add uAPI")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Acked-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 include/uapi/drm/panthor_drm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index ad9a70afea6c..3a76c4f2882b 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -296,6 +296,9 @@ struct drm_panthor_gpu_info {
 	/** @as_present: Bitmask encoding the number of address-space exposed by the MMU. */
 	__u32 as_present;
 
+	/** @pad0: MBZ. */
+	__u32 pad0;
+
 	/** @shader_present: Bitmask encoding the shader cores exposed by the GPU. */
 	__u64 shader_present;
 
-- 
2.49.0

[PATCH v3 2/2] drm/panthor: Fix the user MMIO offset logic for emulators

[Boris Brezillon]

Currently, we pick the MMIO offset based on the size of the pgoff_t
type seen by the process that manipulates the FD, such that a 32-bit
process can always map the user MMIO ranges. But this approach doesn't
work well for emulators like FEX, where the emulator is a 64-bit binary
which might be executing 32-bit code. In that case, the kernel thinks
it's the 64-bit process and assumes DRM_PANTHOR_USER_MMIO_OFFSET_64BIT
is in use, but the UMD library expects DRM_PANTHOR_USER_MMIO_OFFSET_32BIT,
because it can't mmap() anything above the pgoff_t size.

In order to solve that, we need a way to explicitly set the user MMIO
offset from the UMD, such that the kernel doesn't have to guess it
from the TIF_32BIT flag set on user thread. We keep the old behavior
if DRM_PANTHOR_SET_USER_MMIO_OFFSET is never called.

Changes in v2:
- Drop the lock/immutable fields and allow SET_USER_MMIO_OFFSET
  requests to race with mmap() requests
- Don't do the is_user_mmio_offset test twice in panthor_mmap()
- Improve the uAPI docs

Changes in v3:
- Bump to version 1.5 instead of 1.4 after rebasing
- Add R-bs
- Fix/rephrase comment as suggested by Liviu

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.h | 18 ++++++++
 drivers/gpu/drm/panthor/panthor_drv.c    | 56 +++++++++++++++++-------
 include/uapi/drm/panthor_drm.h           | 38 ++++++++++++++++
 3 files changed, 96 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 465d3ab1b79e..a4ce2a4c626d 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -230,6 +230,24 @@ struct panthor_file {
 	/** @ptdev: Device attached to this file. */
 	struct panthor_device *ptdev;
 
+	/** @user_mmio: User MMIO related fields. */
+	struct {
+		/**
+		 * @offset: Offset used for user MMIO mappings.
+		 *
+		 * This offset should not be used to check the type of mapping
+		 * except in panthor_mmap(). After that point, MMIO mapping
+		 * offsets have been adjusted to match
+		 * DRM_PANTHOR_USER_MMIO_OFFSET and that macro should be used
+		 * instead.
+		 * Make sure this rule is followed at all times, because
+		 * userspace is in control of the offset, and can change the
+		 * value behind our back. Otherwise it can lead to erroneous
+		 * branching happening in kernel space.
+		 */
+		u64 offset;
+	} user_mmio;
+
 	/** @vms: VM pool attached to this file. */
 	struct panthor_vm_pool *vms;
 
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 229b9190f152..3b4c35ce4461 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1379,6 +1379,20 @@ static int panthor_ioctl_bo_set_label(struct drm_device *ddev, void *data,
 	return ret;
 }
 
+static int panthor_ioctl_set_user_mmio_offset(struct drm_device *ddev,
+					      void *data, struct drm_file *file)
+{
+	struct drm_panthor_set_user_mmio_offset *args = data;
+	struct panthor_file *pfile = file->driver_priv;
+
+	if (args->offset != DRM_PANTHOR_USER_MMIO_OFFSET_32BIT &&
+	    args->offset != DRM_PANTHOR_USER_MMIO_OFFSET_64BIT)
+		return -EINVAL;
+
+	WRITE_ONCE(pfile->user_mmio.offset, args->offset);
+	return 0;
+}
+
 static int
 panthor_open(struct drm_device *ddev, struct drm_file *file)
 {
@@ -1396,6 +1410,18 @@ panthor_open(struct drm_device *ddev, struct drm_file *file)
 	}
 
 	pfile->ptdev = ptdev;
+	pfile->user_mmio.offset = DRM_PANTHOR_USER_MMIO_OFFSET;
+
+#ifdef CONFIG_ARM64
+	/*
+	 * With 32-bit systems being limited by the 32-bit representation of
+	 * mmap2's pgoffset field, we need to make the MMIO offset arch
+	 * specific.
+	 */
+	if (test_tsk_thread_flag(current, TIF_32BIT))
+		pfile->user_mmio.offset = DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
+#endif
+
 
 	ret = panthor_vm_pool_create(pfile);
 	if (ret)
@@ -1449,6 +1475,7 @@ static const struct drm_ioctl_desc panthor_drm_driver_ioctls[] = {
 	PANTHOR_IOCTL(TILER_HEAP_DESTROY, tiler_heap_destroy, DRM_RENDER_ALLOW),
 	PANTHOR_IOCTL(GROUP_SUBMIT, group_submit, DRM_RENDER_ALLOW),
 	PANTHOR_IOCTL(BO_SET_LABEL, bo_set_label, DRM_RENDER_ALLOW),
+	PANTHOR_IOCTL(SET_USER_MMIO_OFFSET, set_user_mmio_offset, DRM_RENDER_ALLOW),
 };
 
 static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
@@ -1457,30 +1484,26 @@ static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
 	struct panthor_file *pfile = file->driver_priv;
 	struct panthor_device *ptdev = pfile->ptdev;
 	u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
+	u64 user_mmio_offset;
 	int ret, cookie;
 
 	if (!drm_dev_enter(file->minor->dev, &cookie))
 		return -ENODEV;
 
-#ifdef CONFIG_ARM64
-	/*
-	 * With 32-bit systems being limited by the 32-bit representation of
-	 * mmap2's pgoffset field, we need to make the MMIO offset arch
-	 * specific. This converts a user MMIO offset into something the kernel
-	 * driver understands.
+	/* Adjust the user MMIO offset to match the offset used kernel side.
+	 * We use a local variable with a READ_ONCE() here to make sure
+	 * the user_mmio_offset we use for the is_user_mmio_mapping() check
+	 * hasn't changed when we do the offset adjustment.
 	 */
-	if (test_tsk_thread_flag(current, TIF_32BIT) &&
-	    offset >= DRM_PANTHOR_USER_MMIO_OFFSET_32BIT) {
-		offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT -
-			  DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
+	user_mmio_offset = READ_ONCE(pfile->user_mmio.offset);
+	if (offset >= user_mmio_offset) {
+		offset -= user_mmio_offset;
+		offset += DRM_PANTHOR_USER_MMIO_OFFSET;
 		vma->vm_pgoff = offset >> PAGE_SHIFT;
-	}
-#endif
-
-	if (offset >= DRM_PANTHOR_USER_MMIO_OFFSET)
 		ret = panthor_device_mmap_io(ptdev, vma);
-	else
+	} else {
 		ret = drm_gem_mmap(filp, vma);
+	}
 
 	drm_dev_exit(cookie);
 	return ret;
@@ -1584,6 +1607,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
  *       - adds PANTHOR_GROUP_PRIORITY_REALTIME priority
  * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag
  * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl
+ * - 1.5 - adds DRM_PANTHOR_SET_USER_MMIO_OFFSET ioctl
  */
 static const struct drm_driver panthor_drm_driver = {
 	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
@@ -1597,7 +1621,7 @@ static const struct drm_driver panthor_drm_driver = {
 	.name = "panthor",
 	.desc = "Panthor DRM driver",
 	.major = 1,
-	.minor = 4,
+	.minor = 5,
 
 	.gem_create_object = panthor_gem_create_object,
 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index 3a76c4f2882b..e1f43deb7eca 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -130,6 +130,20 @@ enum drm_panthor_ioctl_id {
 
 	/** @DRM_PANTHOR_BO_SET_LABEL: Label a BO. */
 	DRM_PANTHOR_BO_SET_LABEL,
+
+	/**
+	 * @DRM_PANTHOR_SET_USER_MMIO_OFFSET: Set the offset to use as the user MMIO offset.
+	 *
+	 * The default behavior is to pick the MMIO offset based on the size of the pgoff_t
+	 * type seen by the process that manipulates the FD, such that a 32-bit process can
+	 * always map the user MMIO ranges. But this approach doesn't work well for emulators
+	 * like FEX, where the emulator is an 64-bit binary which might be executing 32-bit
+	 * code. In that case, the kernel thinks it's the 64-bit process and assumes
+	 * DRM_PANTHOR_USER_MMIO_OFFSET_64BIT is in use, but the UMD library expects
+	 * DRM_PANTHOR_USER_MMIO_OFFSET_32BIT, because it can't mmap() anything above the
+	 * pgoff_t size.
+	 */
+	DRM_PANTHOR_SET_USER_MMIO_OFFSET,
 };
 
 /**
@@ -1001,6 +1015,28 @@ struct drm_panthor_bo_set_label {
 	__u64 label;
 };
 
+/**
+ * struct drm_panthor_set_user_mmio_offset - Arguments passed to
+ * DRM_IOCTL_PANTHOR_SET_USER_MMIO_OFFSET
+ *
+ * This ioctl is only really useful if you want to support userspace
+ * CPU emulation environments where the size of an unsigned long differs
+ * between the host and the guest architectures.
+ */
+struct drm_panthor_set_user_mmio_offset {
+	/**
+	 * @offset: User MMIO offset to use.
+	 *
+	 * Must be either DRM_PANTHOR_USER_MMIO_OFFSET_32BIT or
+	 * DRM_PANTHOR_USER_MMIO_OFFSET_64BIT.
+	 *
+	 * Use DRM_PANTHOR_USER_MMIO_OFFSET (which selects OFFSET_32BIT or
+	 * OFFSET_64BIT based on the size of an unsigned long) unless you
+	 * have a very good reason to overrule this decision.
+	 */
+	__u64 offset;
+};
+
 /**
  * DRM_IOCTL_PANTHOR() - Build a Panthor IOCTL number
  * @__access: Access type. Must be R, W or RW.
@@ -1045,6 +1081,8 @@ enum {
 		DRM_IOCTL_PANTHOR(WR, TILER_HEAP_DESTROY, tiler_heap_destroy),
 	DRM_IOCTL_PANTHOR_BO_SET_LABEL =
 		DRM_IOCTL_PANTHOR(WR, BO_SET_LABEL, bo_set_label),
+	DRM_IOCTL_PANTHOR_SET_USER_MMIO_OFFSET =
+		DRM_IOCTL_PANTHOR(WR, SET_USER_MMIO_OFFSET, set_user_mmio_offset),
 };
 
 #if defined(__cplusplus)
-- 
2.49.0

Re: [PATCH v3 0/2] drm/panthor: Fix panthor+FEX-Emu

[Boris Brezillon]

On Fri,  6 Jun 2025 10:09:30 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello,
> 
> This is an attempt a couple bugs exposed by FEX-Emu. The first one
> is pretty trivial and should be uncontroversial, since it's just
> a missing padding field in one of our uAPI structs. We are getting
> away with it on arm32 because of the alignment rules provided by
> the Arm ABI, but x86 has relaxed constraints for u64 fields, and
> this bug is definitely hit when running a 32-bit x86 mesa binary
> under FEX Emu.
> 
> The second fix is addressing a problem we have because FEX-Emu is
> an aarch64 process executing 32-bit x86 code, meaning the check
> we do on the is-32bit-task check we do to figure out the MMIO
> offset seen by the user won't work. In order to fix that, we add
> an ioctl to let the user explicitly set this offset. The offset
> can only be set early on, if no MMIO range has been mapped before.
> 
> With those, and the mesa MR at [1], I managed to run a 32-bit x86
> glmark2 through FEX without using the host mesa (if we were to use
> the thunked mesa lib, both the kernel and mesa would use
> MMIO_OFFSET_64BIT, and the problem doesn't exist anymore).
> 
> Regards,
> 
> Boris
> 
> Changes in v2:
> - Simplify the logic in patch2 to have a lockless solution that's
>   still safe for what we need
> 
> Changes in v3:
> - Fix a comment
> 
> [1]https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34573
> 
> Boris Brezillon (2):
>   drm/panthor: Add missing explicit padding in drm_panthor_gpu_info
>   drm/panthor: Fix the user MMIO offset logic for emulators

Queued to drm-misc-next.

> 
>  drivers/gpu/drm/panthor/panthor_device.h | 18 ++++++++
>  drivers/gpu/drm/panthor/panthor_drv.c    | 56 +++++++++++++++++-------
>  include/uapi/drm/panthor_drm.h           | 41 +++++++++++++++++
>  3 files changed, 99 insertions(+), 16 deletions(-)
>