[Intel-gfx] [PATCH] drm/i915: Fix possible overflow when recording semaphore states.

[Ben Widawsky]

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 at intel.com>
> Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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)

Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

P.S.
I'd be in favor of adding BUG_ON(idx >= I915_NUM_RINGS) before return in
intel_ring_sync_index().

-- 
Ben Widawsky, Intel Open Source Technology Center