[Intel-gfx] [PATCH 5/6] [v3] drm/i915: Use the new vm [un]bind functions

[Chris Wilson]

On Wed, Sep 18, 2013 at 09:11:56AM -0700, Ben Widawsky wrote:
> On Wed, Sep 18, 2013 at 04:59:01PM +0100, Chris Wilson wrote:
> > On Wed, Sep 18, 2013 at 08:48:45AM -0700, Ben Widawsky wrote:
> > > On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote:
> > > > On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote:
> > > > > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote:
> > > > > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote:
> > > > > > > > The code does
> > > > > > > > 
> > > > > > > > 	exec_start = i915_gem_obj_offset(batch_obj, vm) +
> > > > > > > > 			args->batch_start_offset;
> > > > > > > > 	exec_len = args->batch_len;
> > > > > > > > 	...
> > > > > > > > 	ret = ring->dispatch_execbuffer(ring,
> > > > > > > > 					exec_start, exec_len,
> > > > > > > > 					flags);
> > > > > > > > 	if (ret)
> > > > > > > > 		goto err;
> > > > > > > > 
> > > > > > > > So we lookup the address of the batch buffer in the wrong vm for
> > > > > > > > I915_DISPATCH_SECURE.
> > > > > > > > -Chris
> > > > > > > > 
> > > > > > > 
> > > > > > > But this is very easily solved, no?
> > > > > > > 
> > > > > > > http://cgit.freedesktop.org/~bwidawsk/drm-intel/tree/drivers/gpu/drm/i915/i915_gem_execbuffer.c?h=ppgtt#n1083
> > > > > > 
> > > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is
> > > > > > going to use the ggtt for this execution...
> > > > > > -Chris
> > > > > > 
> > > > > > -- 
> > > > > > Chris Wilson, Intel Open Source Technology Centre
> > > > > 
> > > > > I guess your use of the term, "CS" here is a bit confusing to me, since
> > > > > in my head the CS better do whatever we tell it to do.
> > > > 
> > > > Exactly, we tell the CS to use the ggtt for the SECURE batch (at least
> > > > on some generations).
> > > >  
> > > > > If you're trying to say, "just because batch_obj has a ggtt binding;
> > > > > that doesn't necessarily mean it's the one to use at dispatch." I think
> > > > > that statement is true, but it's still pretty simple to solve, just use
> > > > > the I915_DISPATCH_SECURE flag to check instead of
> > > > > obj->has_global_gtt_mapping. Right?
> > > > 
> > > > Yes. With the same caveat that it may change.
> > > 
> > > What may change? Dispatch secure always means use the GGTT offset, does
> > > it not? Or do you think we'll want privileged batches running from the
> > > PPGTT? If the latter is true, why ever use GGTT?
> > 
> > The security bit is already independent from the use-ppgtt bit. With
> > Haswell it should be possible to execute a privileged batch buffer from
> > a ppgtt address, right? In which case we would not need to allocate a
> > GGTT entry.
> >  
> 
> Right, that was my point. But I *still* fail to see how your earlier
> request does anything to help this along. The decision can still easily
> be made at any given time with the I915_DISPATCH_SECURE flag, and per
> platform driver policy. Say, if you wanted HSW to always run privileged
> batches out of PPGTT. OTOH, if we need to pass a flag down to specify
> which address space to execute the batch out of, maybe some more hoops
> need to be jumped through. I don't see a reason to do this however, and
> if we want to support IVB, we have to support GGTT execution anyway - so
> I'm not really sure of a benefit to building in support for PPGTT
> privileged execution.
> 
> 
> > > > > I'm really sorry about being so dense here.
> > > > > 
> > > > > As a side note, I tried really hard to think of how we could end up with
> > > > > a ggtt mapping for batch_obj, and not want to use that one. I'm not
> > > > > actually sure it's possible, but I can't prove it as such, so I'm
> > > > > willing to assume it is possible. Excluding SNB, so few objects actually
> > > > > will get a ggtt mapping, I don't believe any of them should be reused
> > > > > for a batch BO - however, IGT can probably make it happen.
> > > > 
> > > > It's trivial for a batch to end up with a ggtt entry - userspace can
> > > > just access it through the GTT. Or any bo prior to reusing it as a
> > > > batch.
> > > > -Chris
> > > 
> > > Trivial, perhaps on the gtt mapping. It's not really relevant, but is
> > > any userspace currently doing that? As for BO reuse, that's a separate
> > > problem - are we handing back BOs with their mappings intact? That seems
> > > like a security problem.
> > 
> > *Userspace* caches its bo with the mappings intact.
> > -Chris
> 
> Yes, this seems like a potential (albeit small) problem to me if we (the
> kernel) arbitrarily upgrade BOs to a GGTT mapping. I guess everything
> running privileged is trusted though, so we don't need to worry about
> the unintentional global BOs being snooped. It does sort of seem to
> circumvent real PPGTT to some extent though if the global mappings
> linger.
> 
> Let me state that at this point of the thread, I am lost. Do you still
> want the original change you asked for? I still don't understand, or see
> a reason for not just quirking away with a quick check (which I'll state
> again doesn't even matter until patches which haven't yet been written
> are posted) - but I clearly haven't been able to convince you; and
> nobody else is stepping in.

Yes, I want the bug in the code fixed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre