| Message ID | 1493214013-15580-7-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 13bf099..4b764b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -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. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > } > > static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl) > { > bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT; > bdw_gmch_ctl &= BDW_GMCH_GMS_MASK; > - return bdw_gmch_ctl << 25; /* 32 MB units */ > + return (size_t)bdw_gmch_ctl << 25; /* 32 MB units */ > } > > static size_t chv_get_stolen_size(u16 gmch_ctrl) > @@ -2598,11 +2598,11 @@ static size_t chv_get_stolen_size(u16 gmch_ctrl) > * 0x17 to 0x1d: 4MB increments start at 36MB > */ > if (gmch_ctrl < 0x11) > - return gmch_ctrl << 25; > + return (size_t)gmch_ctrl << 25; > else if (gmch_ctrl < 0x17) > - return (gmch_ctrl - 0x11 + 2) << 22; > + return (size_t)(gmch_ctrl - 0x11 + 2) << 22; > else > - return (gmch_ctrl - 0x17 + 9) << 22; > + return (size_t)(gmch_ctrl - 0x17 + 9) << 22; > } > > static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl) > @@ -2611,10 +2611,10 @@ static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl) > gen9_gmch_ctl &= BDW_GMCH_GMS_MASK; > > if (gen9_gmch_ctl < 0xf0) > - return gen9_gmch_ctl << 25; /* 32 MB units */ > + return (size_t)gen9_gmch_ctl << 25; /* 32 MB units */ > else > /* 4MB increments starting at 0xf0 for 4MB */ > - return (gen9_gmch_ctl - 0xf0 + 1) << 22; > + return (size_t)(gen9_gmch_ctl - 0xf0 + 1) << 22; > } > > static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size) > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 13bf099..4b764b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -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 */ } static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl) { bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT; bdw_gmch_ctl &= BDW_GMCH_GMS_MASK; - return bdw_gmch_ctl << 25; /* 32 MB units */ + return (size_t)bdw_gmch_ctl << 25; /* 32 MB units */ } static size_t chv_get_stolen_size(u16 gmch_ctrl) @@ -2598,11 +2598,11 @@ static size_t chv_get_stolen_size(u16 gmch_ctrl) * 0x17 to 0x1d: 4MB increments start at 36MB */ if (gmch_ctrl < 0x11) - return gmch_ctrl << 25; + return (size_t)gmch_ctrl << 25; else if (gmch_ctrl < 0x17) - return (gmch_ctrl - 0x11 + 2) << 22; + return (size_t)(gmch_ctrl - 0x11 + 2) << 22; else - return (gmch_ctrl - 0x17 + 9) << 22; + return (size_t)(gmch_ctrl - 0x17 + 9) << 22; } static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl) @@ -2611,10 +2611,10 @@ static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl) gen9_gmch_ctl &= BDW_GMCH_GMS_MASK; if (gen9_gmch_ctl < 0xf0) - return gen9_gmch_ctl << 25; /* 32 MB units */ + return (size_t)gen9_gmch_ctl << 25; /* 32 MB units */ else /* 4MB increments starting at 0xf0 for 4MB */ - return (gen9_gmch_ctl - 0xf0 + 1) << 22; + return (size_t)(gen9_gmch_ctl - 0xf0 + 1) << 22; } static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
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> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)