Re: [PATCH v2] drm/i915: prevent out of range pt in the PDE macros (take 3)

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	Michel Thierry <michel.thierry@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915: prevent out of range pt in the PDE macros (take 3)
Date: Tue, 6 Oct 2015 12:09:51 +0200	[thread overview]
Message-ID: <20151006100951.GD3383@phenom.ffwll.local> (raw)
In-Reply-To: <20151006094348.GF26237@nuc-i3427.alporthouse.com>

On Tue, Oct 06, 2015 at 10:43:48AM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 10:38:22AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 05, 2015 at 05:59:50PM +0100, Michel Thierry wrote:
> > > On 10/5/2015 5:36 PM, Dave Gordon wrote:
> > > >On 02/10/15 14:16, Michel Thierry wrote:
> > > >>We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> > > >>range pt in gen6_for_each_pde").
> > > >>
> > > >>But the static analyzer still complains that, just before we break due
> > > >>to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> > > >>iter value that is bigger than I915_PDES. Of course, this isn't really
> > > >>a problem since no one uses pt outside the macro. Still, every single
> > > >>new usage of the macro will create a new issue for us to mark as a
> > > >>false positive.
> > > >>
> > > >>Also, Paulo re-started the discussion a while ago [1], but didn't end up
> > > >>implemented.
> > > >>
> > > >>In order to "solve" this "problem", this patch takes the ideas from
> > > >>Chris and Dave, but that check would change the desired behavior of the
> > > >>code, because the object (for example pdp->page_directory[iter]) can be
> > > >>null during init/alloc, and C would take this as false, breaking the for
> > > >>loop immediately.
> > > >>
> > > >>This has been already verified with "static analysis tools".
> > > >>
> > > >>[1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> > > >>
> > > >>v2: Make it a single statement, while preventing the common subexpression
> > > >>elimination (Chris)
> > > >>
> > > >>Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > >>Cc: Dave Gordon <david.s.gordon@intel.com>
> > > >>Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> 
> > Yeah, since ?: is a ternary operator parsing implicitly adds the () in the
> > middle and always parses it as a ? (b) : c. If lower-level operators in
> > the middle could split the ternary operator then it would result in
> > parsing fail (sinc ? without the : is just useless). So lgtm. Someone
> > willing to smack an r-b onto the patch?
> 
> I think it's good enough.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.

> Something to consider is that ppgtt_insert is 10x slower than
> ppgtt_clear, and that some workloads (admittedly not 48b!) spend a
> disproportionate amount of time changing PTE. If you have ideas for
> spending up insertion, feel free to experiment!

Hm, where do we waste all that time? 10x slower is pretty impressive since
on a quick look I can only see the sg table walk as the additional bit of
memory traversals insert does on top of clear ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx