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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> }
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
