[PATCH 1/4] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[PATCH 1/4] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[Dave Gordon]

Two different sets of flag bits are stored in the 'flags' member of a
'struct drm_i915_gem_exec_object2', and they're defined in two different
source files, increasing the risk of an accidental clash.

Some flags in this field are supplied by the user; these are defined in
i915_drm.h, and they start from the LSB and work up.

Other flags are defined in i915_gem_execbuffer, for internal use within
that file only; they start from the MSB and work down.

So here we add a compile-time check that the two sets of flags do not
overlap, which would cause all sorts of confusion.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++++----
 include/uapi/drm/i915_drm.h                | 11 ++++++-----
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6f4f2a6..08bc37c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -34,10 +34,11 @@
 #include <linux/dma_remapping.h>
 #include <linux/uaccess.h>
 
-#define  __EXEC_OBJECT_HAS_PIN (1<<31)
-#define  __EXEC_OBJECT_HAS_FENCE (1<<30)
-#define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
-#define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
+#define  __EXEC_OBJECT_HAS_PIN		(1<<31)
+#define  __EXEC_OBJECT_HAS_FENCE	(1<<30)
+#define  __EXEC_OBJECT_NEEDS_MAP	(1<<29)
+#define  __EXEC_OBJECT_NEEDS_BIAS	(1<<28)
+#define  __EXEC_OBJECT_INTERNAL_FLAGS (0xf<<28) /* all of the above */
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
@@ -1009,6 +1010,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	unsigned invalid_flags;
 	int i;
 
+	/* INTERNAL flags must not overlap with external ones */
+	BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & ~__EXEC_OBJECT_UNKNOWN_FLAGS);
+
 	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
 	if (USES_FULL_PPGTT(dev))
 		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index a5524cc..80df3b2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -692,12 +692,13 @@ struct drm_i915_gem_exec_object2 {
 	 */
 	__u64 offset;
 
-#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
-#define EXEC_OBJECT_NEEDS_GTT	(1<<1)
-#define EXEC_OBJECT_WRITE	(1<<2)
+#define EXEC_OBJECT_NEEDS_FENCE		 (1<<0)
+#define EXEC_OBJECT_NEEDS_GTT		 (1<<1)
+#define EXEC_OBJECT_WRITE		 (1<<2)
 #define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
-#define EXEC_OBJECT_PINNED	(1<<4)
-#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PINNED<<1)
+#define EXEC_OBJECT_PINNED		 (1<<4)
+/* All remaining bits are MBZ and RESERVED FOR FUTURE USE */
+#define __EXEC_OBJECT_UNKNOWN_FLAGS	(-(EXEC_OBJECT_PINNED<<1))
 	__u64 flags;
 
 	__u64 rsvd1;
-- 
1.9.1

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

[PATCH 2/4] drm/i915: clarify eb_get_batch()

[Dave Gordon]

It may not be obvious, but the current execbuffer2 ioctl interface
requires that the buffer object containing the batch-to-be-executed
be the LAST entry in the exec2_list[] array (I expected it to be
the first!).

To clarify this, we can replace the rather obscure construct
	"list_entry(eb->vmas.prev, ...)"
in eb_get_batch() with the equivalent but more explicit
	"list_last_entry(&eb->vmas,...)"
and of course add an explanatory comment.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 08bc37c..bc97670 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1354,7 +1354,8 @@ static bool only_mappable_for_reloc(unsigned int flags)
 static struct drm_i915_gem_object *
 eb_get_batch(struct eb_vmas *eb)
 {
-	struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list);
+	/* The batch is always the LAST item in the VMA list */
+	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
 
 	/*
 	 * SNA is doing fancy tricks with compressing batch buffers, which leads
-- 
1.9.1

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

[PATCH 3/4] drm/i915: refactor eb_get_batch()

[Dave Gordon]

Precursor for fix to secure batch execution. We will need to be able to
retrieve the batch VMA (as well as the batch itself) from the eb list,
so this patch extracts that part of eb_get_batch() into a separate
function, and moves both parts to a more logical place in the file, near
where the eb list is created.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 50 +++++++++++++++++-------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index bc97670..3a60146 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -186,6 +186,35 @@ struct eb_vmas {
 	return ret;
 }
 
+static inline struct i915_vma *
+eb_get_batch_vma(struct eb_vmas *eb)
+{
+	/* The batch is always the LAST item in the VMA list */
+	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
+
+	return vma;
+}
+
+static struct drm_i915_gem_object *
+eb_get_batch(struct eb_vmas *eb)
+{
+	struct i915_vma *vma = eb_get_batch_vma(eb);
+
+	/*
+	 * SNA is doing fancy tricks with compressing batch buffers, which leads
+	 * to negative relocation deltas. Usually that works out ok since the
+	 * relocate address is still positive, except when the batch is placed
+	 * very low in the GTT. Ensure this doesn't happen.
+	 *
+	 * Note that actual hangs have only been observed on gen7, but for
+	 * paranoia do it everywhere.
+	 */
+	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
+		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
+
+	return vma->obj;
+}
+
 static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
 {
 	if (eb->and < 0) {
@@ -1351,27 +1380,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	return file_priv->bsd_ring;
 }
 
-static struct drm_i915_gem_object *
-eb_get_batch(struct eb_vmas *eb)
-{
-	/* The batch is always the LAST item in the VMA list */
-	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
-
-	/*
-	 * SNA is doing fancy tricks with compressing batch buffers, which leads
-	 * to negative relocation deltas. Usually that works out ok since the
-	 * relocate address is still positive, except when the batch is placed
-	 * very low in the GTT. Ensure this doesn't happen.
-	 *
-	 * Note that actual hangs have only been observed on gen7, but for
-	 * paranoia do it everywhere.
-	 */
-	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
-		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
-
-	return vma->obj;
-}
-
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
-- 
1.9.1

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

[PATCH 4/4] drm/i915: fix relocation of secure buffers

[Dave Gordon]

There is a problem with the relocation of batches submitted with the
I915_EXEC_SECURE flag: although the batch itself will be mapped into the
GGTT, any relocations referring to it will use its address in the PPGTT,
which almost certainly won't be the same.

Hence a batch containing an MI_BATCH_BUFFER_START instruction that
references another part of the same batchbuffer will run correctly
in unprivileged mode, but will fail with a random jump when executed
in privileged mode.

This patch fixes the issue by changing eb_lookup_vmas() to take TWO
address space specifiers, one a new one for the batch itself and the
existing one used for all other buffer objects in the list.

This does not address the known limitation on batches *promoted* to
secure mode by the command parser, which are not allowed to contain
MI_BATCH_BUFFER_START or various other opcodes.

Discovered-by: Miguel Reche <miguel.reche@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Miguel Reche <miguel.reche@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3a60146..c0b4361 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -95,17 +95,19 @@ struct eb_vmas {
 	       struct drm_i915_gem_exec_object2 *exec,
 	       const struct drm_i915_gem_execbuffer2 *args,
 	       struct i915_address_space *vm,
+	       struct i915_address_space *vmb,
 	       struct drm_file *file)
 {
 	struct drm_i915_gem_object *obj;
 	struct list_head objects;
+	int n_obj = args->buffer_count;
 	int i, ret;
 
 	INIT_LIST_HEAD(&objects);
 	spin_lock(&file->table_lock);
 	/* Grab a reference to the object and release the lock so we can lookup
 	 * or create the VMA without using GFP_ATOMIC */
-	for (i = 0; i < args->buffer_count; i++) {
+	for (i = 0; i < n_obj; i++) {
 		obj = to_intel_bo(idr_find(&file->object_idr, exec[i].handle));
 		if (obj == NULL) {
 			spin_unlock(&file->table_lock);
@@ -128,14 +130,17 @@ struct eb_vmas {
 	}
 	spin_unlock(&file->table_lock);
 
-	i = 0;
-	while (!list_empty(&objects)) {
+	for (i = 0; !list_empty(&objects); --n_obj, ++i) {
 		struct i915_vma *vma;
 
 		obj = list_first_entry(&objects,
 				       struct drm_i915_gem_object,
 				       obj_exec_link);
 
+		/* Switch to vmb for the last item */
+		if (n_obj == 1)
+			vm = vmb;
+
 		/*
 		 * NOTE: We can leak any vmas created here when something fails
 		 * later on. But that's no issue since vma_unbind can deal with
@@ -164,7 +169,6 @@ struct eb_vmas {
 			hlist_add_head(&vma->exec_node,
 				       &eb->buckets[handle & eb->and]);
 		}
-		++i;
 	}
 
 	return 0;
@@ -861,7 +865,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 				  struct intel_context *ctx)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *vmb;
 	struct i915_vma *vma;
 	bool need_relocs;
 	int *reloc_offset;
@@ -869,6 +873,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	unsigned count = args->buffer_count;
 
 	vm = list_first_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
+	vmb = eb_get_batch_vma(eb)->vm;
 
 	/* We may process another execbuffer during the unlock... */
 	while (!list_empty(&eb->vmas)) {
@@ -939,7 +944,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 
 	/* reacquire the objects */
 	eb_reset(eb);
-	ret = eb_lookup_vmas(eb, exec, args, vm, file);
+	ret = eb_lookup_vmas(eb, exec, args, vm, vmb, file);
 	if (ret)
 		goto err;
 
@@ -1452,7 +1457,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx;
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *vmb;
 	struct i915_execbuffer_params params_master; /* XXX: will be removed later */
 	struct i915_execbuffer_params *params = &params_master;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
@@ -1520,6 +1525,12 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	else
 		vm = &ggtt->base;
 
+	/* Secure batches must live in GGTT */
+	if (dispatch_flags & I915_DISPATCH_SECURE)
+		vmb = &dev_priv->ggtt.base;
+	else
+		vmb = vm;
+
 	memset(&params_master, 0x00, sizeof(params_master));
 
 	eb = eb_create(args);
@@ -1531,7 +1542,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	}
 
 	/* Look up object handles */
-	ret = eb_lookup_vmas(eb, exec, args, vm, file);
+	ret = eb_lookup_vmas(eb, exec, args, vm, vmb, file);
 	if (ret)
 		goto err;
 
-- 
1.9.1

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

Re: [PATCH 4/4] drm/i915: fix relocation of secure buffers

[Chris Wilson]

On Fri, Apr 15, 2016 at 12:32:57PM +0100, Dave Gordon wrote:
> There is a problem with the relocation of batches submitted with the
> I915_EXEC_SECURE flag: although the batch itself will be mapped into the
> GGTT, any relocations referring to it will use its address in the PPGTT,
> which almost certainly won't be the same.

This is incorrect. We can have and do use secure batches in the GGTT that
use ppGTT self relocations.
-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 4/4] drm/i915: fix relocation of secure buffers

[Dave Gordon]

On 15/04/2016 12:43, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 12:32:57PM +0100, Dave Gordon wrote:
>> There is a problem with the relocation of batches submitted with the
>> I915_EXEC_SECURE flag: although the batch itself will be mapped into the
>> GGTT, any relocations referring to it will use its address in the PPGTT,
>> which almost certainly won't be the same.
> This is incorrect. We can have and do use secure batches in the GGTT that
> use ppGTT self relocations.
> -Chris

No, what I wrote is correct. A batch containing an MI_START_BATCH_BUFFER 
with a relocation description specifying the target as another part of 
the same batch will have the address of the batch in the PPGTT filled in 
there; Miguel has an example to demonstrate this. (And it's obvious from 
the code, relocation is completed before the GGTT mapping is created so 
can't put the GGTT address in the relocated entry).

Therefore, the secure batch cannot jump to another part of the buffer 
and remain in privileged mode.

OTOH it may ALSO be useful for the privileged batch to jump to 
UNPRIVILEGED subroutines, which would require the relocation to provide 
the PPGTT address (although I wouldn't expect that to be the default).

Maybe we need to extend the relocation interface so the batch can choose?

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

✗ Fi.CI.BAT: warning for series starting with [1/4] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
URL   : https://patchwork.freedesktop.org/series/5775/
State : warning

== Summary ==

Series 5775v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5775/revisions/1/mbox/

Test gem_busy:
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
Test gem_storedw_loop:
        Subgroup basic-default:
                pass       -> SKIP       (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                skip       -> PASS       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (skl-i7k-2)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:163  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:203  pass:135  dwarn:0   dfail:0   fail:0   skip:68 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:175  dwarn:2   dfail:0   fail:0   skip:26 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:165  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1915/

d9131d62d18ba94fb3ca019f1156c22b5f4ce23c drm-intel-nightly: 2016y-04m-15d-13h-53m-44s UTC integration manifest
0ec14eb drm/i915: fix relocation of secure buffers
7b9e4bd drm/i915: refactor eb_get_batch()
a224dfa drm/i915: clarify eb_get_batch()
b8aa6b4 drm/i915: compile-time consistency check on __EXEC_OBJECT flags

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