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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.