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