drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

Submitted by Michel Thierry on April 30, 2015, 1:59 p.m.

Details

Message ID 1430402374-16855-1-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry April 30, 2015, 1:59 p.m.
The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
page directories rework allocation" added an overflow warning, but the
mask had an extra 0. Use typo-prone option suggested by Dave instead.

This check will be unnecessary after gen8_alloc_va_range handles more
than 4 PDPs (48b addressing).

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..6d894fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -756,8 +756,8 @@  static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
 
 	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
 
-	/* FIXME: PPGTT container_of won't work for 64b */
-	WARN_ON((start + length) > 0x800000000ULL);
+	/* FIXME: upper bound must not overflow 31 bits  */
+	WARN_ON((start + length) & (~0ULL << 31));
 
 	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
 		if (pd)

Comments

On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> page directories rework allocation" added an overflow warning, but the
> mask had an extra 0. Use typo-prone option suggested by Dave instead.
> 
> This check will be unnecessary after gen8_alloc_va_range handles more
> than 4 PDPs (48b addressing).
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6fae6bd..6d894fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>  
>  	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>  
> -	/* FIXME: PPGTT container_of won't work for 64b */
> -	WARN_ON((start + length) > 0x800000000ULL);
> +	/* FIXME: upper bound must not overflow 31 bits  */
> +	WARN_ON((start + length) & (~0ULL << 31));

Why is it 31 and not 32?

>  
>  	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>  		if (pd)
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>> page directories rework allocation" added an overflow warning, but the
>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>
>> This check will be unnecessary after gen8_alloc_va_range handles more
>> than 4 PDPs (48b addressing).
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 6fae6bd..6d894fc 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>
>>   	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>
>> -	/* FIXME: PPGTT container_of won't work for 64b */
>> -	WARN_ON((start + length) > 0x800000000ULL);
>> +	/* FIXME: upper bound must not overflow 31 bits  */
>> +	WARN_ON((start + length) & (~0ULL << 31));
>
> Why is it 31 and not 32?
>

Right, the check really should be (start + length) >= 0x100000000ULL.

>>
>>   	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>>   		if (pd)
>> --
>> 2.1.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
On 30/04/15 15:33, Michel Thierry wrote:
> On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
>> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>>> page directories rework allocation" added an overflow warning, but the
>>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>>
>>> This check will be unnecessary after gen8_alloc_va_range handles more
>>> than 4 PDPs (48b addressing).
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index 6fae6bd..6d894fc 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -756,8 +756,8 @@ static int
>>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>>
>>>       WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>>
>>> -    /* FIXME: PPGTT container_of won't work for 64b */
>>> -    WARN_ON((start + length) > 0x800000000ULL);
>>> +    /* FIXME: upper bound must not overflow 31 bits  */
>>> +    WARN_ON((start + length) & (~0ULL << 31));
>>
>> Why is it 31 and not 32?
> 
> Right, the check really should be (start + length) >= 0x100000000ULL.

Something with '32' in it might be more obvious and save anyone having
to count the zeroes ... and a comment that also mentioned the limit:

	/* FIXME: for now, upper bound must fit in 32 bits  */

	WARN_ON((start + length) >= (1ULL << 32))
	WARN_ON((start + length) & (~0ULL << 32))
	WARN_ON((start + length) >> 32) != 0)

.Dave.
On 4/30/2015 3:53 PM, Dave Gordon wrote:
> On 30/04/15 15:33, Michel Thierry wrote:
>> On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
>>> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>>>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>>>> page directories rework allocation" added an overflow warning, but the
>>>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>>>
>>>> This check will be unnecessary after gen8_alloc_va_range handles more
>>>> than 4 PDPs (48b addressing).
>>>>
>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> index 6fae6bd..6d894fc 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> @@ -756,8 +756,8 @@ static int
>>>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>>>
>>>>        WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>>>
>>>> -    /* FIXME: PPGTT container_of won't work for 64b */
>>>> -    WARN_ON((start + length) > 0x800000000ULL);
>>>> +    /* FIXME: upper bound must not overflow 31 bits  */
>>>> +    WARN_ON((start + length) & (~0ULL << 31));
>>>
>>> Why is it 31 and not 32?
>>
>> Right, the check really should be (start + length) >= 0x100000000ULL.
>
> Something with '32' in it might be more obvious and save anyone having
> to count the zeroes ... and a comment that also mentioned the limit:
>
> 	/* FIXME: for now, upper bound must fit in 32 bits  */
>
> 	WARN_ON((start + length) >= (1ULL << 32))
> 	WARN_ON((start + length) & (~0ULL << 32))
> 	WARN_ON((start + length) >> 32) != 0)

Yes, I was planning to just replace _31_ with _32_ and avoid all those 
zeroes...

>
> .Dave.
>
On Thu, Apr 30, 2015 at 03:53:29PM +0100, Dave Gordon wrote:
> On 30/04/15 15:33, Michel Thierry wrote:
> > On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
> >> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> >>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> >>> page directories rework allocation" added an overflow warning, but the
> >>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
> >>>
> >>> This check will be unnecessary after gen8_alloc_va_range handles more
> >>> than 4 PDPs (48b addressing).
> >>>
> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>> Cc: Dave Gordon <david.s.gordon@intel.com>
> >>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> index 6fae6bd..6d894fc 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> @@ -756,8 +756,8 @@ static int
> >>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
> >>>
> >>>       WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
> >>>
> >>> -    /* FIXME: PPGTT container_of won't work for 64b */
> >>> -    WARN_ON((start + length) > 0x800000000ULL);
> >>> +    /* FIXME: upper bound must not overflow 31 bits  */
> >>> +    WARN_ON((start + length) & (~0ULL << 31));
> >>
> >> Why is it 31 and not 32?
> > 
> > Right, the check really should be (start + length) >= 0x100000000ULL.
> 
> Something with '32' in it might be more obvious and save anyone having
> to count the zeroes ... and a comment that also mentioned the limit:
> 
> 	/* FIXME: for now, upper bound must fit in 32 bits  */
> 
> 	WARN_ON((start + length) >= (1ULL << 32))

This would match the '1ULL<<32' used in the ppgtt init.

> 	WARN_ON((start + length) & (~0ULL << 32))
> 	WARN_ON((start + length) >> 32) != 0)
> 
> .Dave.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6299
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(18)      TIMEOUT(1)PASS(1)
*BYT  igt@gem_exec_parse@bitmasks      FAIL(1)PASS(7)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(10)PASS(9)      TIMEOUT(2)
Note: You need to pay more attention to line start with '*'
On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> page directories rework allocation" added an overflow warning, but the
> mask had an extra 0. Use typo-prone option suggested by Dave instead.

I think you mean "less typo-prone" here :)


Kind regards, David Weinehall