From: Chris Wilson <chris@chris-wilson.co.uk>
To: Ben Widawsky <ben@bwidawsk.net>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/2] drm/i915: read/write ioctls for userspace
Date: Mon, 04 Apr 2011 18:14:48 +0100 [thread overview]
Message-ID: <b9dded$iiff9n@orsmga002.jf.intel.com> (raw)
In-Reply-To: <1301702089-25644-2-git-send-email-ben@bwidawsk.net>
Comments inline.
On Fri, 1 Apr 2011 16:54:48 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Straight forward IOCTLs which allow userspace to read and write
> registers. This is needed because on newer generation products,
> registers may become inaccessible due to specific power modes.
>
> To manage this, created a set of register ranges for the different
> products which can give fine grained control over what we permit
> userspace to read and write. This list was created by hand, so expect
> issues/bugs.
>
> The fields in the calls allow register widths to be defined, although
> currently this is not used by any products. There is also a rsvd field
> which can be used for giving a list of register reads/writes to perform
> in the future.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 189 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 21 +++++
> include/drm/i915_drm.h | 30 ++++++
> 3 files changed, 240 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..30cd576 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1846,6 +1846,174 @@ out_unlock:
> }
> EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>
> +static struct i915_register_range *
> +get_register_range(struct drm_device *dev, u32 offset, int mode)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct i915_register_range *range = dev_priv->register_map.map;
> + u8 align = dev_priv->register_map.alignment_mask;
> + u32 range_count = dev_priv->register_map.length;
> +
> + if (offset & dev_priv->register_map.alignment_mask)
> + return NULL;
> +
> + if (offset >= dev_priv->register_map.top)
> + return NULL;
> +
> + while(range_count--) {
Missingwhitespace.;-)
Rather than range_count/dev_priv->register_map.length, I would prefer a
sentinel.
> + /* list is assumed to be in order */
> + if (offset < range->base)
> + break;
> +
> + if ( (offset >= range->base) &&
> + (offset + align) <= (range->base + range->size)) {
> + /* assume perms ascend in security (ie. rw is > ro) */
> + if (mode > range->flags)
> + return NULL;
> + else
> + return range;
> + }
> + range++;
> + }
> +
> + return NULL;
> +}
> +
> +static int
> +i915_read_register_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_intel_write_reg *args = data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (args->rsvd != 0)
> + DRM_DEBUG("rsvd field should be zero\n");
No, don't even mention it, just ignore it.
> +
> + if (get_register_range(dev, args->offset, I915_RANGE_RO) == NULL) {
> + args->value = 0xffffffff;
> + return -EINVAL;
> + }
> +
> + mutex_lock(&dev->struct_mutex);
Will be happier with i915_mutex_lock_interruptible() just in case.
> + args->value = (u32)i915_gt_read(dev_priv, args->offset);
And we need to start doing
intel_register_read_get(dev, args->offset);
switch (args->size) {
switch 8: args->value = ioread8(regs+args->offset); break;
...
}
intel_register_read_put(dev, args->offset);
where intel_register_read_get() looks something like
int intel_register_read_get(struct drm_device *dev,
int offset)
{
switch (INTEL_INFO(dev)->gen) {
case 6:
if (offset < 0x40000)
__gen6_gt_force_wake_get(dev);
return 0;
case 5:
case 4:
case 3:
return 0;
}
}
> + mutex_unlock(&dev->struct_mutex);
> +
> + return 0;
> +}
> +
> +static int
> +i915_write_register_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_intel_read_reg *args = data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct i915_register_range *range;
> +
> + if (args->rsvd != 0)
> + DRM_DEBUG("rsvd field should be zero\n");
> +
> + range = get_register_range(dev, args->offset, I915_RANGE_RW);
> + if (!range)
> + return -EINVAL;
> +
> + mutex_lock(&dev->struct_mutex);
> + DRM_INFO("User space write %x %x\n", args->offset, (u32)args->value);
> + range->user_tainted = true;
> + i915_gt_write(dev_priv, args->offset, (u32)args->value);
ret = intel_register_write_get()
if (ret == 0) {
switch (args->size) {
case 8: iowrite8(args->value, regs+args->offset); break;
}
intel_register_write_put()
}
> + mutex_unlock(&dev->struct_mutex);
> +
> + return 0;
> +}
> +
> +struct i915_register_range gen_bwcl_register_map[] = {
static const struct i915_register_range ...
Keep sparse quiet.
> + {0x00000000, 0x00000fff, I915_RANGE_RW},
> + {0x00001000, 0x00000fff, I915_RANGE_RSVD},
> + {0x00002000, 0x00000fff, I915_RANGE_RW},
> + {0x00003000, 0x000001ff, I915_RANGE_RW},
> + {0x00003200, 0x00000dff, I915_RANGE_RW},
> + {0x00004000, 0x000003ff, I915_RANGE_RSVD},
> + {0x00004400, 0x00000bff, I915_RANGE_RSVD},
> + {0x00005000, 0x00000fff, I915_RANGE_RW},
> + {0x00006000, 0x00000fff, I915_RANGE_RW},
> + {0x00007000, 0x000003ff, I915_RANGE_RW},
> + {0x00007400, 0x000014ff, I915_RANGE_RW},
> + {0x00008900, 0x000006ff, I915_RANGE_RSVD},
> + {0x00009000, 0x00000fff, I915_RANGE_RSVD},
> + {0x0000a000, 0x00000fff, I915_RANGE_RW},
> + {0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> + {0x00010000, 0x00003fff, I915_RANGE_RW},
> + {0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> + {0x00030000, 0x0000ffff, I915_RANGE_RW},
> + {0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> + {0x00060000, 0x0000ffff, I915_RANGE_RW},
> + {0x00070000, 0x00002fff, I915_RANGE_RW},
> + {0x00073000, 0x00000fff, I915_RANGE_RW},
> + {0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range genx_register_map[] = {
> + {0x00000000, 0x00000fff, I915_RANGE_RW},
> + {0x00001000, 0x00000fff, I915_RANGE_RSVD},
> + {0x00002000, 0x00000fff, I915_RANGE_RW},
> + {0x00003000, 0x000001ff, I915_RANGE_RW},
> + {0x00003200, 0x00000dff, I915_RANGE_RW},
> + {0x00004000, 0x000003ff, I915_RANGE_RW},
> + {0x00004400, 0x00000bff, I915_RANGE_RW},
> + {0x00005000, 0x00000fff, I915_RANGE_RW},
> + {0x00006000, 0x00000fff, I915_RANGE_RW},
> + {0x00007000, 0x000003ff, I915_RANGE_RW},
> + {0x00007400, 0x000014ff, I915_RANGE_RW},
> + {0x00008900, 0x000006ff, I915_RANGE_RSVD},
> + {0x00009000, 0x00000fff, I915_RANGE_RSVD},
> + {0x0000a000, 0x00000fff, I915_RANGE_RW},
> + {0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> + {0x00010000, 0x00003fff, I915_RANGE_RW},
> + {0x00014000, 0x0001bfff, I915_RANGE_RSVD},
> + {0x00030000, 0x0000ffff, I915_RANGE_RW},
> + {0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> + {0x00060000, 0x0000ffff, I915_RANGE_RW},
> + {0x00070000, 0x00002fff, I915_RANGE_RW},
> + {0x00073000, 0x00000fff, I915_RANGE_RW},
> + {0x00074000, 0x0000bfff, I915_RANGE_RSVD}
> +};
> +
> +struct i915_register_range gen6_gt_register_map[] = {
> + {0x00000000, 0x00000fff, I915_RANGE_RW},
> + {0x00001000, 0x00000fff, I915_RANGE_RSVD},
> + {0x00002000, 0x00000fff, I915_RANGE_RW},
> + {0x00003000, 0x000001ff, I915_RANGE_RW},
> + {0x00003200, 0x00000dff, I915_RANGE_RW},
> + {0x00004000, 0x00000fff, I915_RANGE_RW},
> + {0x00005000, 0x0000017f, I915_RANGE_RW},
> + {0x00005180, 0x00000e7f, I915_RANGE_RW},
> + {0x00006000, 0x00001fff, I915_RANGE_RW},
> + {0x00008000, 0x000007ff, I915_RANGE_RW},
> + {0x00008800, 0x000000ff, I915_RANGE_RSVD},
> + {0x00008900, 0x000006ff, I915_RANGE_RW},
> + {0x00009000, 0x00000fff, I915_RANGE_RSVD},
> + {0x0000a000, 0x00000fff, I915_RANGE_RW},
> + {0x0000b000, 0x00004fff, I915_RANGE_RSVD},
> + {0x00010000, 0x00001fff, I915_RANGE_RW},
> + {0x00012000, 0x000003ff, I915_RANGE_RW},
> + {0x00012400, 0x00000bff, I915_RANGE_RW},
> + {0x00013000, 0x00000fff, I915_RANGE_RW},
> + {0x00014000, 0x00000fff, I915_RANGE_RW},
> + {0x00015000, 0x0000cfff, I915_RANGE_RW},
> + {0x00022000, 0x00000fff, I915_RANGE_RW},
> + {0x00023000, 0x00000fff, I915_RANGE_RSVD},
> + {0x00024000, 0x00000fff, I915_RANGE_RW},
> + {0x00025000, 0x0000afff, I915_RANGE_RSVD},
> + {0x00030000, 0x0000ffff, I915_RANGE_RW},
> + {0x00040000, 0x0001ffff, I915_RANGE_RSVD},
> + {0x00060000, 0x0000ffff, I915_RANGE_RW},
> + {0x00070000, 0x00002fff, I915_RANGE_RW},
> + {0x00073000, 0x00000fff, I915_RANGE_RW},
> + {0x00074000, 0x0008bfff, I915_RANGE_RSVD},
> + {0x00100000, 0x00007fff, I915_RANGE_RW},
> + {0x00108000, 0x00037fff, I915_RANGE_RSVD},
> + {0x00140000, 0x0003ffff, I915_RANGE_RW}
> +};
> +
> /**
> * Tells the intel_ips driver that the i915 driver is now loaded, if
> * IPS got loaded first.
> @@ -2062,6 +2230,25 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
> ips_ping_for_i915_load();
>
> + if (IS_GEN6(dev)) {
> + dev_priv->register_map.map = gen6_gt_register_map;
> + dev_priv->register_map.length =
> + ARRAY_SIZE(gen6_gt_register_map);
> + dev_priv->register_map.top = 0x180000;
> + } else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev)) {
> + dev_priv->register_map.map = gen_bwcl_register_map;
> + dev_priv->register_map.length =
> + ARRAY_SIZE(gen_bwcl_register_map);
> + dev_priv->register_map.top = 0x80000;
> + } else {
> + dev_priv->register_map.map = genx_register_map;
Wow, didn't expect the gen5 register map to match gen2.
> + dev_priv->register_map.length =
> + ARRAY_SIZE(genx_register_map);
> + dev_priv->register_map.top = 0x80000;
> + }
> +
> + dev_priv->register_map.alignment_mask = 0x3;
> +
> return 0;
>
> out_gem_unload:
> @@ -2276,6 +2463,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED),
> DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> + DRM_IOCTL_DEF_DRV(I915_READ_REGISTER, i915_read_register_ioctl, DRM_UNLOCKED),
> + DRM_IOCTL_DEF_DRV(I915_WRITE_REGISTER, i915_write_register_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
> };
>
> int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..355f008 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -703,6 +703,14 @@ typedef struct drm_i915_private {
> struct intel_fbdev *fbdev;
>
> struct drm_property *broadcast_rgb_property;
> +
> + /* User visible register map */
> + struct {
> + struct i915_register_range *map;
> + u32 length;
As noted we can remove this. I hope.
> + u32 top;
> + u8 alignment_mask;
> + } register_map;
> } drm_i915_private_t;
>
> struct drm_i915_gem_object {
> @@ -1385,4 +1393,17 @@ static inline void i915_gt_write(struct drm_i915_private *dev_priv,
> __gen6_gt_wait_for_fifo(dev_priv);
> I915_WRITE(reg, val);
> }
> +
> +/* Register range interface */
> +struct i915_register_range {
> + u32 base;
> + u32 size;
> + u8 flags;
> + bool user_tainted;
Save a puppy, merge this into flags.
> +};
> +
> +#define I915_RANGE_RSVD (0<<0)
> +#define I915_RANGE_RO (1<<0)
> +#define I915_RANGE_RW (1<<1)
Mind renaming this as I915_RANGE_READ, I915_RANGE_WRITE and use them as
distinct permission bits and then I915_RANGE_RW is (I915_RANGE_READ |
I915_RANGE_WRITE). Yes, there are the occasional write-only registers out
there.
> +
> #endif
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index c4d6dbf..9398cb2 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -198,6 +198,8 @@ typedef struct _drm_i915_sarea {
> #define DRM_I915_OVERLAY_PUT_IMAGE 0x27
> #define DRM_I915_OVERLAY_ATTRS 0x28
> #define DRM_I915_GEM_EXECBUFFER2 0x29
> +#define DRM_I915_READ_REGISTER 0x2a
> +#define DRM_I915_WRITE_REGISTER 0x2b
>
> #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -239,6 +241,8 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_GEM_MADVISE DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MADVISE, struct drm_i915_gem_madvise)
> #define DRM_IOCTL_I915_OVERLAY_PUT_IMAGE DRM_IOW(DRM_COMMAND_BASE + DRM_IOCTL_I915_OVERLAY_ATTRS, struct drm_intel_overlay_put_image)
> #define DRM_IOCTL_I915_OVERLAY_ATTRS DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> +#define DRM_IOCTL_I915_READ_REGISTER DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_READ_REGISTER, struct drm_intel_read_reg)
> +#define DRM_IOCTL_I915_WRITE_REGISTER DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_WRITE_REGISTER, struct drm_intel_write_reg)
>
> /* Allow drivers to submit batchbuffers directly to hardware, relying
> * on the security mechanisms provided by hardware.
> @@ -844,4 +848,30 @@ struct drm_intel_overlay_attrs {
> __u32 gamma5;
> };
>
> +struct drm_intel_read_reg {
> + /* register offset to read */
> + __u32 offset;
> +
> + /* register size, RFU */
> + __u8 size;
Make this a _u32 for alignment's sake. Leave a note saying that are a
spare 26 bits if you want ;-)
> +
> + /* return value, high 4 bytes RFU */
> + __u64 value;
> +
> + __u64 rsvd;
> +};
> +
> +struct drm_intel_write_reg {
> + /* register offset to read */
> + __u32 offset;
> +
> + /* register size, RFU */
> + __u8 size;
> +
> + /* value to write, high 4 bytes RFU */
> + __u64 value;
> +
> + __u64 rsvd;
> +};
> +
> #endif /* _I915_DRM_H_ */
> --
> 1.7.3.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Chris Wilson, Intel Open Source Technology Centre
next prev parent reply other threads:[~2011-04-04 17:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-01 1:31 [PATCH] drm/i915: read/write IOCTLs Ben Widawsky
2011-04-01 1:39 ` [PATCH v2] " Ben Widawsky
2011-04-01 23:54 ` Read/Write IOCTL compromise Ben Widawsky
2011-04-06 21:38 ` No more read/write ioctls Ben Widawsky
2011-04-08 17:10 ` Eric Anholt
2011-04-08 17:29 ` Chris Wilson
2011-04-01 23:54 ` [PATCH v3 1/2] drm/i915: read/write ioctls for userspace Ben Widawsky
2011-04-04 17:14 ` Chris Wilson [this message]
2011-04-05 1:30 ` Read/Write ioctls Ben Widawsky
2011-04-05 1:30 ` [PATCH v4] drm/i915: read/write ioctls for userspace Ben Widawsky
2011-04-01 23:54 ` [PATCH v3 2/2] drm/i915: debugfs for register write taint Ben Widawsky
2011-04-01 6:36 ` [PATCH] drm/i915: read/write IOCTLs Chris Wilson
2011-04-01 7:06 ` Ben Widawsky
2011-04-01 7:32 ` Chris Wilson
2011-04-01 18:51 ` Eric Anholt
2011-04-02 6:46 ` Chris Wilson
2011-04-02 15:16 ` Ben Widawsky
2011-04-04 1:35 ` Ben Widawsky
2011-04-04 7:36 ` Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='b9dded$iiff9n@orsmga002.jf.intel.com' \
--to=chris@chris-wilson.co.uk \
--cc=ben@bwidawsk.net \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).