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

[Ben Widawsky <ben@bwidawsk.net>]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center