[RFC] Fixing up relocation of secure buffers?

Commit Message

Dave Gordon April 11, 2016, 6:47 p.m. UTC

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 <[e-mail]>

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

Comments

Chris Wilson April 11, 2016, 8:52 p.m. UTC | #1

On Mon, Apr 11, 2016 at 07:47:12PM +0100, Dave Gordon wrote:
> 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 :(

Tricky. Problem here is that even some relocations will read from the
ppgtt but a few will read from the ggtt. I was going to suggest a second
relocation pass for the ggtt secure batch - but we can't tell which will
be which. We do have the NEEDS_GGTT execobject flag, but that is not
allowed on full-ppgtt (atm). We could look for a read_domain = COMMAND,
but that will likely end up in confusion - though at first glance that
seems good enough.

So a second relocation pass for the secure batch applying the GGTT
offset to self-relocations with read_domain == COMMAND? (Being sure not
to apply the pass to promoted cmdparser batches.) The
relocation[].presumed_offsets should be set to -1, as should the
execobject.offset since the relocations are then inconsistent.
-Chris

Patch

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;