[PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[Chris Wilson]

Useful sanity check that the vma is legal to bind inside the address
space.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 635f2635b1f2..68e66ffb95e8 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -107,6 +107,8 @@ vma_create(struct drm_i915_gem_object *obj,
 		}
 	}
 
+	GEM_BUG_ON(vma->size > vm->total);
+
 	if (i915_is_ggtt(vm)) {
 		GEM_BUG_ON(overflows_type(vma->size, u32));
 		vma->fence_size = i915_gem_fence_size(vm->i915, vma->size,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Reject an attempt to create a GGTT vma larger than the GTT

[Chris Wilson]

For accessing large objects through the global GTT we enforce the use of
partial views, and sanity check that the vma->size doesn't overflow and
fits within the address space. To prevent 4GiB triggering the assert,
catch the attempt to create the full sized vma early.

Testcase: igt/gem_exec_reloc/gtt-32
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b2f8ac1386a2..99ff686dd640 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3679,6 +3679,12 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
 
 	lockdep_assert_held(&obj->base.dev->struct_mutex);
 
+	/* Disallow a single VMA to occupy the entire GGTT, and catch any
+	 * attempt to create a normal VMA larger than the GGTT.
+	 */
+	if (!view && obj->base.size >= vm->total)
+		return ERR_PTR(-ENOSPC);
+
 	vma = i915_vma_instance(obj, vm, view);
 	if (IS_ERR(vma))
 		return vma;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Useful sanity check that the vma is legal to bind inside the address
> space.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_vma.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 635f2635b1f2..68e66ffb95e8 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -107,6 +107,8 @@ vma_create(struct drm_i915_gem_object *obj,
>  		}
>  	}
>  
> +	GEM_BUG_ON(vma->size > vm->total);
> +
>  	if (i915_is_ggtt(vm)) {
>  		GEM_BUG_ON(overflows_type(vma->size, u32));
>  		vma->fence_size = i915_gem_fence_size(vm->i915, vma->size,
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[Joonas Lahtinen]

On to, 2017-01-19 at 12:58 +0000, Chris Wilson wrote:
> Useful sanity check that the vma is legal to bind inside the address
> space.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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

Re: [PATCH 2/2] drm/i915: Reject an attempt to create a GGTT vma larger than the GTT

[Joonas Lahtinen]

On to, 2017-01-19 at 12:58 +0000, Chris Wilson wrote:
> For accessing large objects through the global GTT we enforce the use of
> partial views, and sanity check that the vma->size doesn't overflow and
> fits within the address space. To prevent 4GiB triggering the assert,
> catch the attempt to create the full sized vma early.
> 
> Testcase: igt/gem_exec_reloc/gtt-32
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -3679,6 +3679,12 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>  
>  	lockdep_assert_held(&obj->base.dev->struct_mutex);
>  
> +	/* Disallow a single VMA to occupy the entire GGTT, and catch any
> +	 * attempt to create a normal VMA larger than the GGTT.
> +	 */
> +	if (!view && obj->base.size >= vm->total)
> +		return ERR_PTR(-ENOSPC);

Too sloppy. How many bytes less than the full size are OK?

Change the test to be > vm->total and then it's better.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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

Re: [PATCH 2/2] drm/i915: Reject an attempt to create a GGTT vma larger than the GTT

[Chris Wilson]

On Thu, Jan 19, 2017 at 03:18:03PM +0200, Joonas Lahtinen wrote:
> On to, 2017-01-19 at 12:58 +0000, Chris Wilson wrote:
> > For accessing large objects through the global GTT we enforce the use of
> > partial views, and sanity check that the vma->size doesn't overflow and
> > fits within the address space. To prevent 4GiB triggering the assert,
> > catch the attempt to create the full sized vma early.
> > 
> > Testcase: igt/gem_exec_reloc/gtt-32
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > @@ -3679,6 +3679,12 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> >  
> >  	lockdep_assert_held(&obj->base.dev->struct_mutex);
> >  
> > +	/* Disallow a single VMA to occupy the entire GGTT, and catch any
> > +	 * attempt to create a normal VMA larger than the GGTT.
> > +	 */
> > +	if (!view && obj->base.size >= vm->total)
> > +		return ERR_PTR(-ENOSPC);
> 
> Too sloppy. How many bytes less than the full size are OK?

None.
 
> Change the test to be > vm->total and then it's better.

The rejection at vm->total is to prevent the overflow.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Assert that the vma->fence_size doesn't overflow and wraparound

[Chris Wilson]

A large object may need an invalid fence larger than u32 and so wrap. No
hardware can support such large fences, so it is safe to leave the check
as an assert.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 68e66ffb95e8..3597e3c01342 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -115,6 +115,7 @@ vma_create(struct drm_i915_gem_object *obj,
 						      i915_gem_object_get_tiling(obj),
 						      i915_gem_object_get_stride(obj));
 		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_MIN_ALIGNMENT));
+		GEM_BUG_ON(vma->fence_size < vma->size); /* overflow */
 
 		vma->fence_alignment = i915_gem_fence_alignment(vm->i915, vma->size,
 								i915_gem_object_get_tiling(obj),
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Assert that created vma has a whole number of pages

[Chris Wilson]

VMA (and their objects) are supposed to composed of whole pages. Add an
assert to catch any invalid construct when we create the VMA.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 68e66ffb95e8..57243bc68b86 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -108,6 +108,7 @@ vma_create(struct drm_i915_gem_object *obj,
 	}
 
 	GEM_BUG_ON(vma->size > vm->total);
+	GEM_BUG_ON(!IS_ALIGNED(vma->size, I915_GTT_PAGE_SIZE));
 
 	if (i915_is_ggtt(vm)) {
 		GEM_BUG_ON(overflows_type(vma->size, u32));
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[Chris Wilson]

On Thu, Jan 19, 2017 at 12:58:46PM +0000, Chris Wilson wrote:
> Useful sanity check that the vma is legal to bind inside the address
> space.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 635f2635b1f2..68e66ffb95e8 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -107,6 +107,8 @@ vma_create(struct drm_i915_gem_object *obj,
>  		}
>  	}
>  
> +	GEM_BUG_ON(vma->size > vm->total);

Seconds thoughts, it is probably wiser to bail here with
ERR_PTR(-E2BIG); We don't check in execbuf and this stops from having to
write patch 2 as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert that the vma->fence_size doesn't overflow and wraparound

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> A large object may need an invalid fence larger than u32 and so wrap. No
> hardware can support such large fences, so it is safe to leave the check
> as an assert.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_vma.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 68e66ffb95e8..3597e3c01342 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -115,6 +115,7 @@ vma_create(struct drm_i915_gem_object *obj,
>  						      i915_gem_object_get_tiling(obj),
>  						      i915_gem_object_get_stride(obj));
>  		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_MIN_ALIGNMENT));
> +		GEM_BUG_ON(vma->fence_size < vma->size); /* overflow */
>  
>  		vma->fence_alignment = i915_gem_fence_alignment(vm->i915, vma->size,
>  								i915_gem_object_get_tiling(obj),
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert that created vma has a whole number of pages

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> VMA (and their objects) are supposed to composed of whole pages. Add an
> assert to catch any invalid construct when we create the VMA.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_vma.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 68e66ffb95e8..57243bc68b86 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -108,6 +108,7 @@ vma_create(struct drm_i915_gem_object *obj,
>  	}
>  
>  	GEM_BUG_ON(vma->size > vm->total);
> +	GEM_BUG_ON(!IS_ALIGNED(vma->size, I915_GTT_PAGE_SIZE));
>  
>  	if (i915_is_ggtt(vm)) {
>  		GEM_BUG_ON(overflows_type(vma->size, u32));
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Assert the vma can fit within the address space

[Joonas Lahtinen]

On to, 2017-01-19 at 14:30 +0000, Chris Wilson wrote:
> On Thu, Jan 19, 2017 at 12:58:46PM +0000, Chris Wilson wrote:
> > 
> > Useful sanity check that the vma is legal to bind inside the address
> > space.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> > +	GEM_BUG_ON(vma->size > vm->total);
> 
> Seconds thoughts, it is probably wiser to bail here with
> ERR_PTR(-E2BIG); We don't check in execbuf and this stops from having to
> write patch 2 as well.

I had similar thoughts, so slap my R-b for that (with same condition).

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