[01/03] drm/i915: add a context parameter to {en, dis}able zero address mapping
diff mbox

Message ID 20150520140013.GB13974@boom
State New
Headers show

Commit Message

David Weinehall May 20, 2015, 2 p.m. UTC
Export a new context parameter that can be set/queried through the
context_{get,set}param ioctls.  This parameter is passed as a context
flag and decides whether or not a GPU address mapping is allowed to
be made at address zero.  The default is to allow such mappings.

Signed-off-by: David Weinehall <david.weinehall@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |    5 +++++
 drivers/gpu/drm/i915/i915_gem_context.c    |   11 +++++++++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   13 +++++++++----
 include/uapi/drm/i915_drm.h                |    1 +
 4 files changed, 26 insertions(+), 4 deletions(-)

Comments

Chris Wilson May 28, 2015, 2:39 p.m. UTC | #1
On Wed, May 20, 2015 at 05:00:13PM +0300, David Weinehall wrote:
> Export a new context parameter that can be set/queried through the
> context_{get,set}param ioctls.  This parameter is passed as a context
> flag and decides whether or not a GPU address mapping is allowed to
> be made at address zero.  The default is to allow such mappings.

Please revert this piece of unreviewed API.

The most obvious objection is what value of bias the kernel should be
using.

Then given that what value does this hold over and above userspace
specifying the layout they want?
-Chris
Daniel Vetter May 28, 2015, 3:52 p.m. UTC | #2
On Thu, May 28, 2015 at 03:39:26PM +0100, Chris Wilson wrote:
> On Wed, May 20, 2015 at 05:00:13PM +0300, David Weinehall wrote:
> > Export a new context parameter that can be set/queried through the
> > context_{get,set}param ioctls.  This parameter is passed as a context
> > flag and decides whether or not a GPU address mapping is allowed to
> > be made at address zero.  The default is to allow such mappings.
> 
> Please revert this piece of unreviewed API.
> 
> The most obvious objection is what value of bias the kernel should be
> using.
> 
> Then given that what value does this hold over and above userspace
> specifying the layout they want?

Yeah it would be redundant with softpinning. But that's only for ocl2, and
this is apparently needed for for ocl1.x already. Hence imo it's ok to
pull this ahead a bit, even if redundant.
-Daniel
Chris Wilson May 28, 2015, 4:56 p.m. UTC | #3
On Thu, May 28, 2015 at 05:52:21PM +0200, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 03:39:26PM +0100, Chris Wilson wrote:
> > On Wed, May 20, 2015 at 05:00:13PM +0300, David Weinehall wrote:
> > > Export a new context parameter that can be set/queried through the
> > > context_{get,set}param ioctls.  This parameter is passed as a context
> > > flag and decides whether or not a GPU address mapping is allowed to
> > > be made at address zero.  The default is to allow such mappings.
> > 
> > Please revert this piece of unreviewed API.
> > 
> > The most obvious objection is what value of bias the kernel should be
> > using.
> > 
> > Then given that what value does this hold over and above userspace
> > specifying the layout they want?
> 
> Yeah it would be redundant with softpinning. But that's only for ocl2, and
> this is apparently needed for for ocl1.x already. Hence imo it's ok to
> pull this ahead a bit, even if redundant.

Look at the interface:

CONTEXT_NO_ZEROMAP:
- it has nothing to do with mapping, it only concerns execbuf object
  location
- what is a sensible bias? I certainly do not wish for the hack we have
  right now to become permanent (as this patch makes it).
- it can still easily fail given a kernel that operates on pinned
  objects when not in full-ppgtt

For a context parameter, something like
  CONTEXT_MM_START, CONTEXT_MM_END
that limited the drm_mm range manager for the context (downside is that
such would only be available for full-ppgtt, but really full-ppgtt is
mandatory for this in the first place otherwise it *is* exploitable -
for it not requires a compiler that wouldn't need the NULL pointer to be
all 0). The advantage of that parameter is that it is useful in igt
testing, and even allows for limited rollout of 48bit full-ppgtt, vital
given the workarounds that userspace must buy into before enabling.

And then there is "int flags;". Are we really expecting to use the sign
bit on this bitmask?
-Chris
David Weinehall May 29, 2015, 8:18 a.m. UTC | #4
On Thu, May 28, 2015 at 05:56:25PM +0100, Chris Wilson wrote:
> On Thu, May 28, 2015 at 05:52:21PM +0200, Daniel Vetter wrote:
> > On Thu, May 28, 2015 at 03:39:26PM +0100, Chris Wilson wrote:
> > > On Wed, May 20, 2015 at 05:00:13PM +0300, David Weinehall wrote:
> > > > Export a new context parameter that can be set/queried through the
> > > > context_{get,set}param ioctls.  This parameter is passed as a context
> > > > flag and decides whether or not a GPU address mapping is allowed to
> > > > be made at address zero.  The default is to allow such mappings.
> > > 
> > > Please revert this piece of unreviewed API.
> > > 
> > > The most obvious objection is what value of bias the kernel should be
> > > using.
> > > 
> > > Then given that what value does this hold over and above userspace
> > > specifying the layout they want?
> > 
> > Yeah it would be redundant with softpinning. But that's only for ocl2, and
> > this is apparently needed for for ocl1.x already. Hence imo it's ok to
> > pull this ahead a bit, even if redundant.
> 
> Look at the interface:
> 
> CONTEXT_NO_ZEROMAP:
> - it has nothing to do with mapping, it only concerns execbuf object
>   location

If you want the option renamed I'd be happy to do so, just suggest a
better name.

> - what is a sensible bias? I certainly do not wish for the hack we have
>   right now to become permanent (as this patch makes it).
> - it can still easily fail given a kernel that operates on pinned
>   objects when not in full-ppgtt
> 

I find it weird that you complain that this solution only works with
full ppgtt, then suggest an alternative solution that would only be
available for full ppgtt...

As far as solving the non-ppgtt case, I cannot see any obvious solution
except the original suggestion that was shot down quite quickly
(offsetting all allocations so none of them ever ends up at 0).

> For a context parameter, something like
>   CONTEXT_MM_START, CONTEXT_MM_END
> that limited the drm_mm range manager for the context (downside is that
> such would only be available for full-ppgtt, but really full-ppgtt is
> mandatory for this in the first place otherwise it *is* exploitable -
> for it not requires a compiler that wouldn't need the NULL pointer to be
> all 0). The advantage of that parameter is that it is useful in igt
> testing, and even allows for limited rollout of 48bit full-ppgtt, vital
> given the workarounds that userspace must buy into before enabling.
> 
I still don't see what attack vector you see that does not already
exist in the current code.

Also, what would be the use cases for a generic range limiting
interface, and wouldn't that force userspace to know unnecessarily much
about mm internals?

> And then there is "int flags;". Are we really expecting to use the sign
> bit on this bitmask?

No, that was simply a brainfart on my behalf; thanks for pointing it
out.


Kind regards, David

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index acfa4fc93803..421640a53dd8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -781,11 +781,15 @@  struct i915_ctx_hang_stats {
 
 /* This must match up with the value previously used for execbuf2.rsvd1. */
 #define DEFAULT_CONTEXT_HANDLE 0
+
+#define CONTEXT_NO_ZEROMAP (1<<0)
 /**
  * struct intel_context - as the name implies, represents a context.
  * @ref: reference count.
  * @user_handle: userspace tracking identity for this context.
  * @remap_slice: l3 row remapping information.
+ * @flags: context specific flags:
+ *         CONTEXT_NO_ZEROMAP: do not allow mapping things to page 0.
  * @file_priv: filp associated with this context (NULL for global default
  *	       context).
  * @hang_stats: information about the role of this context in possible GPU
@@ -802,6 +806,7 @@  struct intel_context {
 	struct kref ref;
 	int user_handle;
 	uint8_t remap_slice;
+	int flags;
 	struct drm_i915_file_private *file_priv;
 	struct i915_ctx_hang_stats hang_stats;
 	struct i915_hw_ppgtt *ppgtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5a47eb5e3c5d..10bd0cfe34ad 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -902,6 +902,9 @@  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_CONTEXT_PARAM_BAN_PERIOD:
 		args->value = ctx->hang_stats.ban_period_seconds;
 		break;
+	case I915_CONTEXT_PARAM_NO_ZEROMAP:
+		args->value = ctx->flags & CONTEXT_NO_ZEROMAP;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -939,6 +942,14 @@  int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 		else
 			ctx->hang_stats.ban_period_seconds = args->value;
 		break;
+	case I915_CONTEXT_PARAM_NO_ZEROMAP:
+		if (args->size) {
+			ret = -EINVAL;
+		} else {
+			ctx->flags &= ~CONTEXT_NO_ZEROMAP;
+			ctx->flags |= args->value ? CONTEXT_NO_ZEROMAP : 0;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 560c79a8a43d..6cfc716d7186 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -676,6 +676,7 @@  eb_vma_misplaced(struct i915_vma *vma)
 static int
 i915_gem_execbuffer_reserve(struct intel_engine_cs *ring,
 			    struct list_head *vmas,
+			    struct intel_context *ctx,
 			    bool *need_relocs)
 {
 	struct drm_i915_gem_object *obj;
@@ -698,6 +699,9 @@  i915_gem_execbuffer_reserve(struct intel_engine_cs *ring,
 		obj = vma->obj;
 		entry = vma->exec_entry;
 
+		if (ctx->flags & CONTEXT_NO_ZEROMAP)
+			entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
+
 		if (!has_fenced_gpu_access)
 			entry->flags &= ~EXEC_OBJECT_NEEDS_FENCE;
 		need_fence =
@@ -775,7 +779,8 @@  i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 				  struct drm_file *file,
 				  struct intel_engine_cs *ring,
 				  struct eb_vmas *eb,
-				  struct drm_i915_gem_exec_object2 *exec)
+				  struct drm_i915_gem_exec_object2 *exec,
+				  struct intel_context *ctx)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
 	struct i915_address_space *vm;
@@ -861,7 +866,7 @@  i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 		goto err;
 
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -1515,7 +1520,7 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;
-	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, &need_relocs);
+	ret = i915_gem_execbuffer_reserve(ring, &eb->vmas, ctx, &need_relocs);
 	if (ret)
 		goto err;
 
@@ -1525,7 +1530,7 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, args, file, ring,
-								eb, exec);
+								eb, exec, ctx);
 			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 		}
 		if (ret)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 4851d660243c..5ae2b0f98999 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1102,6 +1102,7 @@  struct drm_i915_gem_context_param {
 	__u32 size;
 	__u64 param;
 #define I915_CONTEXT_PARAM_BAN_PERIOD 0x1
+#define I915_CONTEXT_PARAM_NO_ZEROMAP 0x2
 	__u64 value;
 };