drm/i915: Prevent context obj from being corrupted

PATCH In review Ben Widawsky 3 participants 5 messages Mar 26, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Ben Widawsky

author

2014-03-26 02:02 UTC #0 lore raw

PATCH 1/1 drm/i915: Prevent context obj from being corrupted

While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted.

NOTE: This is a slightly different patch than what I posted to Bugzilla.

References: https://bugs.freedesktop.org/show_bug.cgi?id=75724

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c | 56 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h         |  2 +-
 2 files changed, 57 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem_context.c +56 -0

index 6043062..4405a92 100644
@@ -584,6 +584,58 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 	return ctx;
 }
 
+static void
+_ctx_ptes(struct intel_ring_buffer *ring,
+	  struct i915_hw_context *ctx,
+	  bool valid)
+{
+	const size_t ptes  = ctx->obj->base.size >> PAGE_SHIFT;
+	const u32 base = i915_gem_obj_ggtt_offset(ctx->obj);
+	struct sg_page_iter sg_iter;
+	struct i915_address_space *vm = ctx->vm;
+	int i = 0;
+
+	BUG_ON(!i915_gem_obj_is_pinned(ctx->obj));
+
+	if (intel_ring_begin(ring, round_up(ptes * 3, 2))) {
+		DRM_ERROR("Could not protect context object.\n");
+		return;
+	}
+
+	for_each_sg_page(ctx->obj->pages->sgl, &sg_iter, ctx->obj->pages->nents, 0) {
+		u32 pte = vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
+					 ctx->obj->cache_level,
+					 valid);
+		intel_ring_emit(ring, MI_UPDATE_GTT | (1<<22));
+		/* The docs contradict themselves on the offset. They say dword
+		 * offset, yet the low 12 bits MBZ. */
+		intel_ring_emit(ring, (base & PAGE_MASK) + i);
+		intel_ring_emit(ring, pte);
+		i+=PAGE_SIZE;
+	}
+
+	if (i & PAGE_SHIFT)
+		intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+}
+
+static void
+enable_ctx_ptes(struct intel_ring_buffer *ring,
+		struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, true);
+}
+
+static void
+disable_ctx_ptes(struct intel_ring_buffer *ring,
+		 struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, false);
+}
+
 static inline int
 mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,

@@ -602,6 +654,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
+	enable_ctx_ptes(ring, new_context);
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;

@@ -632,6 +686,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	disable_ctx_ptes(ring, new_context);
+
 	return ret;
 }

drivers/gpu/drm/i915/i915_reg.h +1 -1

index 9f9e2b7..d536706 100644
@@ -367,7 +367,7 @@
 #define MI_TOPOLOGY_FILTER      MI_INSTR(0x0D, 0)
 #define MI_LOAD_SCAN_LINES_EXCL MI_INSTR(0x13, 0)
 #define MI_URB_CLEAR            MI_INSTR(0x19, 0)
-#define MI_UPDATE_GTT           MI_INSTR(0x23, 0)
+#define MI_UPDATE_GTT           MI_INSTR(0x23, 1)
 #define MI_CLFLUSH              MI_INSTR(0x27, 0)
 #define MI_REPORT_PERF_COUNT    MI_INSTR(0x28, 0)
 #define   MI_REPORT_PERF_COUNT_GGTT (1<<0)

Signature · 1 line

1.9.1

Daniel Vetter

2014-03-26 06:24 UTC #1 lore raw

Quoted 8 lines · Ben Widawsky

While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted.
NOTE: This is a slightly different patch than what I posted to Bugzilla.
References: https://bugs.freedesktop.org/show_bug.cgi?id=75724
Signed-off-by Ben Widawsky <[email protected]>

We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ...
-Daniel

Quoted 106 lines

---
 drivers/gpu/drm/i915/i915_gem_context.c | 56 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h         |  2 +-
 2 files changed, 57 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem_context.c +56 -0

index 6043062..4405a92 100644
@@ -584,6 +584,58 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 	return ctx;
 }
 
+static void
+_ctx_ptes(struct intel_ring_buffer *ring,
+	  struct i915_hw_context *ctx,
+	  bool valid)
+{
+	const size_t ptes  = ctx->obj->base.size >> PAGE_SHIFT;
+	const u32 base = i915_gem_obj_ggtt_offset(ctx->obj);
+	struct sg_page_iter sg_iter;
+	struct i915_address_space *vm = ctx->vm;
+	int i = 0;
+
+	BUG_ON(!i915_gem_obj_is_pinned(ctx->obj));
+
+	if (intel_ring_begin(ring, round_up(ptes * 3, 2))) {
+		DRM_ERROR("Could not protect context object.\n");
+		return;
+	}
+
+	for_each_sg_page(ctx->obj->pages->sgl, &sg_iter, ctx->obj->pages->nents, 0) {
+		u32 pte = vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
+					 ctx->obj->cache_level,
+					 valid);
+		intel_ring_emit(ring, MI_UPDATE_GTT | (1<<22));
+		/* The docs contradict themselves on the offset. They say dword
+		 * offset, yet the low 12 bits MBZ. */
+		intel_ring_emit(ring, (base & PAGE_MASK) + i);
+		intel_ring_emit(ring, pte);
+		i+=PAGE_SIZE;
+	}
+
+	if (i & PAGE_SHIFT)
+		intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+}
+
+static void
+enable_ctx_ptes(struct intel_ring_buffer *ring,
+		struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, true);
+}
+
+static void
+disable_ctx_ptes(struct intel_ring_buffer *ring,
+		 struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, false);
+}
+
 static inline int
 mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,

@@ -602,6 +654,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
+	enable_ctx_ptes(ring, new_context);
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;

@@ -632,6 +686,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	disable_ctx_ptes(ring, new_context);
+
 	return ret;
 }

drivers/gpu/drm/i915/i915_reg.h +1 -1

index 9f9e2b7..d536706 100644
@@ -367,7 +367,7 @@
 #define MI_TOPOLOGY_FILTER      MI_INSTR(0x0D, 0)
 #define MI_LOAD_SCAN_LINES_EXCL MI_INSTR(0x13, 0)
 #define MI_URB_CLEAR            MI_INSTR(0x19, 0)
-#define MI_UPDATE_GTT           MI_INSTR(0x23, 0)
+#define MI_UPDATE_GTT           MI_INSTR(0x23, 1)
 #define MI_CLFLUSH              MI_INSTR(0x27, 0)
 #define MI_REPORT_PERF_COUNT    MI_INSTR(0x28, 0)
 #define   MI_REPORT_PERF_COUNT_GGTT (1<<0)

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Daniel Vetter

2014-03-26 07:04 UTC #2 lore raw

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Ben Widawsky
While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted.
NOTE: This is a slightly different patch than what I posted to Bugzilla.

Quoted 7 lines

Quoted 2 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=75724
Signed-off-by Ben Widawsky <[email protected]>
We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ...

Actually your patch doesn't accomplish much, since MI_UPDATE_GTT only updates the global gtt, not the aliasing ppgtt. Which means that userspace batches can still happily stomp all over your precious context objects. See comment #5 in that bug report:

https://bugs.freedesktop.org/show_bug.cgi?id=75724#c5

Cheers, Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Ben Widawsky

2014-03-26 15:06 UTC #3 lore raw

Quoted 16 lines · Daniel Vetter

Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Ben Widawsky
While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted.
NOTE: This is a slightly different patch than what I posted to Bugzilla.
Quoted 7 lines
Quoted 2 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=75724
Signed-off-by Ben Widawsky <[email protected]>
We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ...

Quoted 8 lines

Actually your patch doesn't accomplish much, since MI_UPDATE_GTT only updates the global gtt, not the aliasing ppgtt. Which means that userspace batches can still happily stomp all over your precious context objects. See comment #5 in that bug report:
https://bugs.freedesktop.org/show_bug.cgi?id=75724#c5
Cheers, Daniel

Quoting you from comment #5:
"ctx objects should only be bound in the global gtt with aliasing ppgtt."

The bug was not present with full PPGTT.

Quoted 8 lines

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Ben Widawsky

2014-03-26 15:08 UTC #4 lore raw

Quoted 24 lines · Ben Widawsky

Quoted 23 lines · Daniel Vetter
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Ben Widawsky
While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted.
NOTE: This is a slightly different patch than what I posted to Bugzilla.
Quoted 7 lines
Quoted 2 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=75724
Signed-off-by Ben Widawsky <[email protected]>
We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ...
Actually your patch doesn't accomplish much, since MI_UPDATE_GTT only updates the global gtt, not the aliasing ppgtt. Which means that userspace batches can still happily stomp all over your precious context objects. See comment #5 in that bug report:
https://bugs.freedesktop.org/show_bug.cgi?id=75724#c5

Quoted 8 lines

Quoted 1 line
Cheers, Daniel
Quoting you from comment #5:
"ctx objects should only be bound in the global gtt with aliasing ppgtt."
The bug was not present with full PPGTT.

Oh, I see the confusion. I don't necessarily think it's userspace that's corrupting this. I just want to prove it's *not* being corrupted.

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center