[Intel-gfx] [RFC] Fixing up relocation of secure buffers?

[Dave Gordon]

Hi,

background to this is that one of our validation engineers has written
a test that creates a batch that loops by jumping backwards using
MI_BATCH_BUFFER_START to an address earlier in the batchbuffer, with
whatever instruction sequence is being tested inside the loop.

This works perfectly well for normal cases, but in some cases the
instruction to be tested is privileged, so the batch has to be submitted
with the I915_DISPATCH_SECURE flag.

In this case, although the batch executes correctly on the first pass,
the jump backwards doesn't. It appears that the relocation process has
inserted a PPGTT-based address for the target, whereas the opcode in
the batch has the GGTT bit set (as required for jumping to a privileged
batch). The result is effectively a random jump :(

So, I put this little patch together, in which we pass TWO address spaces
to eb_lookup_vmas(), one to be used for the batch (object 0 in the args)
and the other for all other buffer objects. Then we can either pass
the same address space for both (regular non-privileged batch, or h/w
doesn't support PPGTT), or GGTT for the batch and PPGTT for the rest.

That part appears to work, but the relocation is still wrong i.e. using
a PPGTT-based address rather than the VMA associated with object 0. So,
any pointers as to why relocation isn't using the right VM?

Oh yes, also the code in i915_gem_execbuffer_relocate_slow() is a hack.
What would be the right way to recover vm0 and vm in this function? But 
we're not reaching that code yet, AFAIK.

Thanks,
.Dave.

Cc: Miguel Reche <miguel.reche at intel.com>

---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ee61fd..84f590c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -93,6 +93,7 @@ struct eb_vmas {
 eb_lookup_vmas(struct eb_vmas *eb,
 	       struct drm_i915_gem_exec_object2 *exec,
 	       const struct drm_i915_gem_execbuffer2 *args,
+	       struct i915_address_space *vm0,
 	       struct i915_address_space *vm,
 	       struct drm_file *file)
 {
@@ -143,7 +144,7 @@ struct eb_vmas {
 		 * from the (obj, vm) we don't run the risk of creating
 		 * duplicated vmas for the same vm.
 		 */
-		vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
+		vma = i915_gem_obj_lookup_or_create_vma(obj, i ? vm : vm0);
 		if (IS_ERR(vma)) {
 			DRM_DEBUG("Failed to lookup VMA\n");
 			ret = PTR_ERR(vma);
@@ -831,14 +832,15 @@ 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 *vm0, *vm;
 	struct i915_vma *vma;
 	bool need_relocs;
 	int *reloc_offset;
 	int i, total, ret;
 	unsigned count = args->buffer_count;
 
-	vm = list_first_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
+	vm0 = list_first_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
+	vm = list_last_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
 
 	/* We may process another execbuffer during the unlock... */
 	while (!list_empty(&eb->vmas)) {
@@ -909,7 +911,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, vm0, vm, file);
 	if (ret)
 		goto err;
 
@@ -1440,7 +1442,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 *vm0, *vm;
 	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);
@@ -1508,6 +1510,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)
+		vm0 = &dev_priv->ggtt.base;
+	else
+		vm0 = vm;
+
 	memset(&params_master, 0x00, sizeof(params_master));
 
 	eb = eb_create(args);
@@ -1519,7 +1527,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, vm0, vm, file);
 	if (ret)
 		goto err;
 
-- 
1.9.1