| Message ID | 20150520140013.GB13974@boom (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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; };
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(-)