Re: [PATCH 6/8] drm/i915: Sanitize stolen memory size calculation

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Imre Deak" <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/8] drm/i915: Sanitize stolen memory size calculation
Date: Thu, 27 Apr 2017 12:34:18 +0300	[thread overview]
Message-ID: <1493285658.7266.18.camel@linux.intel.com> (raw)
In-Reply-To: <20170426152724.GB30290@intel.com>

On ke, 2017-04-26 at 18:27 +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:11PM +0300, Imre Deak wrote:
> > 
> > On GEN8+ (not counting CHV) the calculation can in theory result in an
> > incorrect sign extension with all upper bits set. In practice this is
> > unlikely to happen since it would require 4GB of stolen memory set
> > aside. For consistency still prevent the sign extension explicitly
> > everywhere.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>

<SNIP>

> > @@ -2577,14 +2577,14 @@ static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
> >  {
> > > >  	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
> > > >  	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
> > > > -	return snb_gmch_ctl << 25; /* 32 MB units */
> > > > +	return (size_t)snb_gmch_ctl << 25; /* 32 MB units */
> 
> So the u16 gets promoted to int, which gets converted to size_t,
> which may be larger than int, and thus things get sign extended.
> 
> Can't happen in the gen6 case actually due to SNB_GMCH_GMS_MASK being
> small enough. But the gen8 case at least looks theoretically possible.
> But having the case everywhere seems like the best way to avoid
> someone copy-pasting the wrong thing when the next variant gets added.

I was about to comment that early-quirks needs to be fixed too, but it
was already fixed when I synchronized the code last time.

That reminded me that I still have the GIT branch to eliminate the code
duplication, which is probably why I didn't revamp the i915 variants.

The trouble is that in early-quirks, pci subsystem functions are
unavailable and the functions are __init, so we'll have to choose
between:

a) code duplication, as we have now
b) move the functions to i915_drm.h as inline with hooks, code size is not
   shrunk, but code duplication is eliminated
c) always have the functions as non __init, even if i915 is disabled
   reduces duplication and kernel size, (well, increases x86 tiny kernel
   size)
c) <insert your suggestion here>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx