Quoting Chris Wilson (2019-12-07 19:01:08)
> The gen7 cmdparser is primarily a promotion-based system to allow access
> to additional registers beyond the HW validation, and allows fallback to
> normal execution of the user batch buffer if valid and requires
> chaining. In the next patch, we will do the cmdparser validation in the
> pipeline asynchronously and so at the point of request construction we
> will not know if we want to execute the privileged and validated batch,
> or the original user batch. The solution employed here is to execute
> both batches, one with raised privileges and one as normal. This is
> because the gen7 MI_BATCH_BUFFER_START command cannot change privilege
> level within a batch and must strictly use the current privilege level
> (or undefined behaviour kills the GPU). So in order to execute the
> original batch, we need a second non-priviledged batch buffer chain from
> the ring, i.e. we need to emit two batches for each user batch. Inside
> the two batches we determine which one should actually execute, we
> provide a conditional trampoline to call the original batch.
It's only a single batch executed twice from different offsets. I would
rephrase the commit message to reflect that.
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
<SNIP>
> #define NUM_EXTRA 2
Looks like BOs, we should have a more descriptive name.
#define NUM_KERNEL_BUFFERS?
> @@ -1985,59 +1970,80 @@ shadow_batch_pin(struct i915_execbuffer *eb, struct drm_i915_gem_object *obj)
> static int eb_parse(struct i915_execbuffer *eb)
> {
> struct intel_engine_pool_node *pool;
> - struct i915_vma *vma;
> + struct i915_vma *shadow, *trampoline;
> + unsigned int len;
> int err;
>
> if (!eb_use_cmdparser(eb))
> return 0;
>
> - pool = intel_engine_get_pool(eb->engine, eb->batch_len);
> + len = eb->batch_len;
> + if (!CMDPARSER_USES_GGTT(eb->i915)) {
> + /*
> + * PPGTT backed shadow buffers must be mapped RO, to prevent
> + * post-scan tampering
> + */
> + if (!eb->context->vm->has_read_only) {
> + DRM_DEBUG("Cannot prevent post-scan tampering without RO capable vm\n");
> + return -EINVAL;
> + }
> + } else {
> + len += 8;
Magic number. #define TRAMPOLINE_SOMETHING ?
> @@ -2089,6 +2095,16 @@ static int eb_submit(struct i915_execbuffer *eb)
> if (err)
> return err;
>
> + if (eb->trampoline) {
> + GEM_BUG_ON(eb->batch_start_offset);
> + err = eb->engine->emit_bb_start(eb->request,
> + eb->trampoline->node.start +
> + eb->batch_len,
> + 8, 0);
Magic 8 detected.
I'd emphasis that we're jumping to the end, either by computing start +
batch_len separately or bringing them to same line.
> @@ -1504,6 +1504,33 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
> }
> } while (1);
>
> + if (!jump_whitelist) { /* setup up the trampoline for chaining */
I think we should hoist the CMDPARSER_USES_GGTT check from
alloc_whitelist function. It's quite misleading now, and I
spent quite some time wondering why we would only do this on
out of memory.
Especially as there is a comment "defer failure until attempted use".
> + cmd = page_mask_bits(shadow->obj->mm.mapping);
> + if (!ret) {
> + cmd += batch_length / sizeof(*cmd);
This could use sharing the offset through variable/helper function
to tie this to be overwriting the trampoline jump.
Helper func maybe to compute the offset of trampoline, even if it
happens to be right at the end.
> + *cmd = MI_BATCH_BUFFER_END;
> + } else {
> + *cmd = MI_BATCH_BUFFER_END;
> + cmd += batch_length / sizeof(*cmd);
Again using the helper function would help tracing that each BB
is jumped to twice, and this is about the second jump.
> +
> + if (ret == -EACCES) {
> + u32 bbs;
> +
> + bbs = MI_BATCH_BUFFER_START;
> + bbs |= MI_BATCH_NON_SECURE_I965;
> + if (IS_HASWELL(engine->i915))
> + bbs |= MI_BATCH_NON_SECURE_HSW;
> +
> + cmd[0] = bbs;
> + cmd[1] = batch_addr;
__{gen6,hsw}_bb_start helper?
With the magics removed this is;
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
