Re: [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 18, 2014 at 07:00:40PM -0700, Ben Widawsky wrote:
> On Fri, Jul 18, 2014 at 02:19:40AM -0700, Rodrigo Vivi wrote:
> > semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
> > This optimization is to remove the ring itself from the list and the logic to do that
> > is at intel_ring_sync_index as below:
> > 
> > /*
> >  * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
> >  * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
> >  * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
> >  * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
> >  * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> > */
> > 
> > v2: Skip when from == to (Damien).
> > v3: avoid computing idx when from == to (Damien).
> >     use ring == to instead of ring->id == to->id (Damien).
> >     use continue instead of return (Rodrigo).
> > v4: avoid all unecessary computation (Damien).
> >     reduce idx to loop scope (Damien).
> > 
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 9faebbc..0b3f694 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> >  					struct intel_engine_cs *ring,
> >  					struct drm_i915_error_ring *ering)
> >  {
> > -	struct intel_engine_cs *useless;
> > +	struct intel_engine_cs *to;
> >  	int i;
> >  
> >  	if (!i915_semaphore_is_enabled(dev_priv->dev))
> > @@ -776,13 +776,20 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> >  						 dev_priv->semaphore_obj,
> >  						 &dev_priv->gtt.base);
> >  
> > -	for_each_ring(useless, dev_priv, i) {
> > -		u16 signal_offset =
> > -			(GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> > -		u32 *tmp = error->semaphore_obj->pages[0];
> > +	for_each_ring(to, dev_priv, i) {
> > +		int idx;
> > +		u16 signal_offset;
> > +		u32 *tmp;
> >  
> > -		ering->semaphore_mboxes[i] = tmp[signal_offset];
> > -		ering->semaphore_seqno[i] = ring->semaphore.sync_seqno[i];
> > +		if (ring == to)
> > +			continue;
> > +
> > +		signal_offset = (GEN8_SIGNAL_OFFSET(ring, i) & PAGE_MASK) / 4;
> > +		tmp = error->semaphore_obj->pages[0];
> > +		idx = intel_ring_sync_index(ring, to);
> > +
> > +		ering->semaphore_mboxes[idx] = tmp[signal_offset];
> > +		ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
> >  	}
> >  }
> >  
> 
> Just elaborate on previous email from your v1, now that you've fixed the
> error state printing, I would have rather not skip the check and instead
> expand the array. This would help us find stray, or unexpected writes
> with either future bugs, or buggy hardware.
> 
> I'm not asking for a v5 (but I did ask you to make a note of what we
> miss in the commit message when I responded to v1... but that's okay
> too). I am simply explaining the earlier mail in case it was unclear. If
> a v5 *does* need to happen anyway, that is still my preference, but I
> don't care too much.
> 
> (Also, I think v2 in your commit message was (Ben), not (Damien) but
> perhaps I missed a conversation somewhere)

We could do this as a follow-up, merged the current version to dinq.
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> P.S.
> I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in
> intel_ring_sync_index().

BUG_ON considered harmful, please use WARN_ON instead. But not sure how
much this is worth it here really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch