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

From: David Weinehall <david.weinehall@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 01/03] drm/i915: add a context parameter to {en, dis}able zero address mapping
Date: Fri, 29 May 2015 11:18:40 +0300	[thread overview]
Message-ID: <20150529081840.GC4009@boom> (raw)
In-Reply-To: <20150528165625.GI19710@nuc-i3427.alporthouse.com>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx