drm/panthor: Fix the user MMIO offset logic for emulators

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

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>
Link: https://lore.kernel.org/r/20250606080932.4140010-3-boris.brezillon@collabora.com
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 465d3ab1b79e01d0fc3f44ac65fe151365f98eb7..a4ce2a4c626dd326e601ec1e61dbd39d661f8218 100644 (file)
--- 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 229b9190f15238711c82cf18a23e1d7090464481..3b4c35ce4461a2286596c291883010538f832bc8 100644 (file)
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1379,6 +1379,20 @@ err_put_obj:
        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 3a76c4f2882b66ff155442e501b3c4317f6cf8ad..e1f43deb7ecab23acb952b9a75e09047eb9e226a 100644 (file)
--- 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)