[PATCH v6 2/2] drm/xe: restore xe_mmio_ioctl as the ioctl handler of the mmio debugfs file
Matt Roper
matthew.d.roper at intel.com
Wed Feb 14 22:06:04 UTC 2024
On Wed, Jan 31, 2024 at 09:59:53PM +0200, Koby Elbaz wrote:
> The drm mmio ioctl handler (xe_mmio_ioctl) wasn't needed anymore,
> and therefore, recently removed.
> Nevertheless, it is now restored for debugging purposes to function
> as the ioctl handler of the 'mmio' debugfs file.
> Note, that the non-admin user's limited mmio access (aka, whitelist),
> was removed being irrelevant anymore, now that a user using debugfs
> must anyway have root permission to use it.
>
> Signed-off-by: Koby Elbaz <kelbaz at habana.ai>
> ---
> Changes in v6:
> - Removed unnecessary header includes.
> - Expanded the scope of security_lock_down check.
> drivers/gpu/drm/xe/xe_debugfs.c | 111 ++++++++++++++++++++++++++-
> drivers/gpu/drm/xe/xe_debugfs_mmio.h | 43 +++++++++++
> 2 files changed, 152 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/xe/xe_debugfs_mmio.h
>
> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index 21b2257aa3a8..cc9d108f7fb2 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -6,13 +6,15 @@
> #include "xe_debugfs.h"
>
> #include <linux/string_helpers.h>
> -
> +#include <linux/security.h>
> #include <drm/drm_debugfs.h>
>
> #include "xe_bo.h"
> #include "xe_device.h"
> #include "xe_gt_debugfs.h"
> #include "xe_step.h"
> +#include "xe_debugfs_mmio.h"
> +#include "xe_mmio.h"
>
> #ifdef CONFIG_DRM_XE_DEBUG
> #include "xe_bo_evict.h"
> @@ -98,8 +100,113 @@ static int forcewake_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> -static long xe_debugfs_mmio_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +static int _xe_debugfs_mmio_ioctl(struct xe_device *xe, struct xe_debugfs_mmio_ioctl_data *args)
> +{
> + struct xe_gt *gt = xe_root_mmio_gt(xe);
This part doesn't look right; this might not wake up the GT that the
specific register you're reading resides within (e.g., a register on
tile0 media GT, or a register on tile1 primary GT).
It seems like we need to either:
- Figure out which GT the register resides within and grab that
specific GT's forcewake domains (or just don't grab forcewake at all
if we determine that this is a non-GT sgunit/soc/display/etc.
register).
- Be lazy and just grab every GT's forcewake in a loop so that we don't
have to put any effort into figuring out what the register is.
> + unsigned int bits_flag, bytes;
> + struct xe_reg reg;
> + int ret = 0;
> +
> + if (XE_IOCTL_DBG(xe, args->extensions) ||
> + XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> + return -EINVAL;
> +
> + if (XE_IOCTL_DBG(xe, args->flags & ~XE_DEBUGFS_MMIO_VALID_FLAGS))
> + return -EINVAL;
> +
> + if (XE_IOCTL_DBG(xe, !(args->flags & XE_DEBUGFS_MMIO_WRITE) && args->value))
> + return -EINVAL;
> +
> + bits_flag = args->flags & XE_DEBUGFS_MMIO_BITS_MASK;
> + bytes = 1 << bits_flag;
> + if (XE_IOCTL_DBG(xe, args->addr + bytes > xe->mmio.size))
> + return -EINVAL;
> +
> + /*
> + * TODO: allow distinction between XE_REG / XE_REG_EXT to
> + * handle registers from both MMIO & MMIO_EXT spaces.
> + */
> +
> + /*
> + * TODO: migrate to xe_gt_mcr to lookup the mmio range and handle
> + * multicast registers. Steering would need uapi extension.
> + */
> + reg = XE_REG(args->addr);
Should there be some kind of sanity check here to make sure userspace
isn't asking for some address past the end of our iomap? Even if the
debugfs isn't available to non-admin users, it's still best if we can
avoid a panic just because someone typo'd an extra digit on the end of a
register offset they were trying to read/write.
> +
> + xe_device_mem_access_get(xe);
> + xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +
> + if (args->flags & XE_DEBUGFS_MMIO_WRITE) {
> + switch (bits_flag) {
> + case XE_DEBUGFS_MMIO_32BIT:
> + if (XE_IOCTL_DBG(xe, args->value > U32_MAX)) {
> + ret = -EINVAL;
> + goto exit;
> + }
> + xe_mmio_write32(gt, reg, args->value);
> + break;
> + default:
> + drm_dbg(&xe->drm, "Invalid MMIO bit size");
> + fallthrough;
> + case XE_DEBUGFS_MMIO_8BIT: /* TODO */
> + case XE_DEBUGFS_MMIO_16BIT: /* TODO */
> + ret = -EOPNOTSUPP;
> + goto exit;
> + }
> + }
> +
> + if (args->flags & XE_DEBUGFS_MMIO_READ) {
> + switch (bits_flag) {
> + case XE_DEBUGFS_MMIO_32BIT:
> + args->value = xe_mmio_read32(gt, reg);
> + break;
> + case XE_DEBUGFS_MMIO_64BIT:
> + args->value = xe_mmio_read64_2x32(gt, reg);
> + break;
> + default:
> + drm_dbg(&xe->drm, "Invalid MMIO bit size");
> + fallthrough;
> + case XE_DEBUGFS_MMIO_8BIT: /* TODO */
> + case XE_DEBUGFS_MMIO_16BIT: /* TODO */
> + ret = -EOPNOTSUPP;
> + }
> + }
> +
> +exit:
> + xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> + xe_device_mem_access_put(xe);
> +
> + return ret;
> +}
> +
> +static long xe_debugfs_mmio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> + struct xe_device *xe = file_inode(filp)->i_private;
> + struct xe_debugfs_mmio_ioctl_data mmio_data;
> + int ret;
> +
> + ret = security_locked_down(LOCKDOWN_PCI_ACCESS);
> + if (ret)
> + return ret;
> +
> + if (cmd == XE_DEBUGFS_MMIO_IOCTL) {
> + ret = copy_from_user(&mmio_data, (struct xe_debugfs_mmio_ioctl_data __user *)arg,
> + sizeof(mmio_data));
> + if (ret)
> + return -EFAULT;
> +
> + ret = _xe_debugfs_mmio_ioctl(xe, &mmio_data);
> + if (ret)
> + return ret;
> +
> + ret = copy_to_user((struct xe_debugfs_mmio_ioctl_data __user *)arg,
> + &mmio_data, sizeof(mmio_data));
> + if (ret)
> + return -EFAULT;
> + } else {
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/xe/xe_debugfs_mmio.h b/drivers/gpu/drm/xe/xe_debugfs_mmio.h
> new file mode 100644
> index 000000000000..1dfb8fa2ce9a
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_debugfs_mmio.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_DEBUGFS_MMIO_H_
> +#define _XE_DEBUGFS_MMIO_H_
> +
> +#include <drm/drm.h>
> +
> +#define XE_DEBUGFS_MMIO_8BIT 0x0
> +#define XE_DEBUGFS_MMIO_16BIT 0x1
> +#define XE_DEBUGFS_MMIO_32BIT 0x2
> +#define XE_DEBUGFS_MMIO_64BIT 0x3
> +#define XE_DEBUGFS_MMIO_BITS_MASK 0x3
> +#define XE_DEBUGFS_MMIO_READ 0x4
> +#define XE_DEBUGFS_MMIO_WRITE 0x8
> +
> +#define XE_DEBUGFS_MMIO_VALID_FLAGS (\
> + XE_DEBUGFS_MMIO_BITS_MASK |\
> + XE_DEBUGFS_MMIO_READ |\
> + XE_DEBUGFS_MMIO_WRITE)
> +
> +struct xe_debugfs_mmio_ioctl_data {
> + /** @extensions: Pointer to the first extension struct, if any */
> + __u64 extensions;
> +
> + __u32 addr;
As noted on the previous patch, making this debugfs per-tile will allow
the address here to be relative to the tile. Otherwise it gets more
challenging to figure out the platform-specific manner to craft an
address here that references the desired register in a non-root tile.
Matt
> +
> + __u32 flags;
> +
> + __u64 value;
> +
> + /** @reserved: Reserved */
> + __u64 reserved[2];
> +};
> +
> +#define XE_DEBUGFS_MMIO_IOCTL_NR 0x00
> +
> +#define XE_DEBUGFS_MMIO_IOCTL \
> + _IOWR(0x20, XE_DEBUGFS_MMIO_IOCTL_NR, struct xe_debugfs_mmio_ioctl_data)
> +
> +#endif /* _XE_DEBUGFS_MMIO_H_ */
> --
> 2.34.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list