Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote:
> On Mon, Nov 25, 2013 at 06:06:18PM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> > > Since the beginning, the functions which try to properly reference the
> > > aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> > > Since the accessors are meant to be global, this will not do.
> > > 
> > > Introduced originally in:
> > > commit a70a3148b0c61cb7c588ea650db785b261b378a3
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Wed Jul 31 16:59:56 2013 -0700
> > > 
> > >     drm/i915: Make proper functions for VMs
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 40d9dcf..bc5c865 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > >  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > >  	struct i915_vma *vma;
> > >  
> > > -	if (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > +	if (!dev_priv->mm.aliasing_ppgtt ||
> > > +	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > 
> > Where's the dereference? gcc is smarter than your average bear.
> > -Chris
> 
> I had assumed: dev_priv->mm.aliasing_ppgtt->base in cases when
> aliasing_ppgtt was NULL. Given that I never actually hit this, I agree
> GCC must be doing something.

Sounds like another discussion on the implementation of offsetof().

> 
> Is such behavior documented somewhere? (forgive the lazy)

IIRC the C standard does say that for &*foo it's as if both & and *
weren't there (and IIRC the same for &foo[x]), but doesn't really
say that kind of thing for &foo->bar. I guess it's a gray area,
and just happens work that way on certain compilers.

In this particular case I think there's one slight issue. If
aliasing_pggtt == NULL, and someone passes in vm == NULL by
accident, then it'll take the branch and use ggtt because
&aliasing_ppgtt->base will be NULL too (due to base being at
offset 0 in the struct). Now, I don't know if a NULL
vm could survive this far into the code, but it it can, then it
might make debugging a bit more "fun".

As a side note, we actually depend on such things in the mode
setting code. Ie. &((struct intel_crtc*)NULL)->base must be
NULL, otherwise interesting bugs would happen.

-- 
Ville Syrjälä
Intel OTC