Re: [PATCH] drm/i915/selftests: Verify whitelist of context registers

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 16/04/2019 10:12, Chris Wilson wrote:
> The RING_NONPRIV allows us to add registers to a whitelist that allows
> userspace to modify them. Ideally such registers should be safe and
> saved within the context such that they do not impact system behaviour
> for other users. This selftest verifies that those registers we do add
> are (a) then writable by userspace and (b) only affect a single client.
> 
> Opens:
> - Is GEN9_SLICE_COMMON_ECO_CHICKEN1 really write-only?
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> Compile fix for writeonly_reg()
> ---
>   .../drm/i915/selftests/intel_workarounds.c    | 320 ++++++++++++++++++
>   1 file changed, 320 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index a363748a7a4f..f40e0937ec96 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -700,6 +700,325 @@ static int live_reset_whitelist(void *arg)
>   	return err;
>   }
>   
> +static int read_whitelisted_registers(struct i915_gem_context *ctx,
> +				      struct intel_engine_cs *engine,
> +				      struct i915_vma *results)
> +{
> +	intel_wakeref_t wakeref;
> +	struct i915_request *rq;
> +	u32 srm, *cs;
> +	int err, i;
> +
> +	rq = ERR_PTR(-ENODEV);
> +	with_intel_runtime_pm(engine->i915, wakeref)
> +		rq = i915_request_alloc(engine, ctx);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
> +
> +	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
> +	if (err)
> +		goto err_req;

Why do you track the vma? There's a wait below and in scrub you even 
flush manually.

> +
> +	srm = MI_STORE_REGISTER_MEM;
> +	if (INTEL_GEN(ctx->i915) >= 8)
> +		srm++;
> +
> +	cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
> +	if (IS_ERR(cs)) {
> +		err = PTR_ERR(cs);
> +		goto err_req;
> +	}
> +
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		u64 offset = results->node.start + sizeof(u32) * i;
> +
> +		*cs++ = srm;
> +		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = lower_32_bits(offset);
> +		*cs++ = upper_32_bits(offset);
> +	}
> +	intel_ring_advance(rq, cs);
> +
> +err_req:
> +	i915_request_add(rq);
> +
> +	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
> +		err = -EIO;
> +
> +	return err;
> +}
> +
> +static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
> +				       struct intel_engine_cs *engine)
> +{
> +	intel_wakeref_t wakeref;
> +	struct i915_request *rq;
> +	struct i915_vma *batch;
> +	int i, err;
> +	u32 *cs;
> +
> +	batch = create_batch(ctx);
> +	if (IS_ERR(batch))
> +		return PTR_ERR(batch);
> +
> +	cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
> +	if (IS_ERR(cs)) {
> +		err = PTR_ERR(cs);
> +		goto err_batch;
> +	}
> +
> +	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = 0xffffffff;
> +	}
> +	*cs++ = MI_BATCH_BUFFER_END;
> +
> +	i915_gem_object_flush_map(batch->obj);
> +	i915_gem_chipset_flush(ctx->i915);
> +
> +	rq = ERR_PTR(-ENODEV);
> +	with_intel_runtime_pm(engine->i915, wakeref)
> +		rq = i915_request_alloc(engine, ctx);
> +	if (IS_ERR(rq))
> +		goto err_unpin;
> +
> +	if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
> +		err = engine->emit_init_breadcrumb(rq);
> +		if (err)
> +			goto err_request;
> +	}
> +
> +	err = engine->emit_bb_start(rq, batch->node.start, 0, 0);

Why is batch needed here when everything else is happy to run from the ring?

> +
> +err_request:
> +	i915_request_add(rq);
> +	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
> +		err = -EIO;
> +
> +err_unpin:
> +	i915_gem_object_unpin_map(batch->obj);
> +err_batch:
> +	i915_vma_unpin_and_release(&batch, 0);
> +	return err;
> +}
> +
> +static bool pardon_reg(struct drm_i915_private *i915, i915_reg_t reg)
> +{
> +	/* Alas, we must pardon some whitelists */
> +	static const struct {
> +		i915_reg_t reg;
> +		unsigned long gen_mask;
> +	} pardon[] = {
> +		{ GEN9_CTX_PREEMPT_REG, INTEL_GEN_MASK(9, 9) },
> +		{ GEN8_L3SQCREG4, INTEL_GEN_MASK(9, 9) },
> +	};
> +	u32 offset = i915_mmio_reg_offset(reg);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pardon); i++) {
> +		if (INTEL_INFO(i915)->gen_mask & pardon[i].gen_mask &&
> +		    i915_mmio_reg_offset(pardon[i].reg) == offset)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int eq_whitelisted_registers(struct i915_vma *A,
> +				    struct i915_vma *B,
> +				    struct intel_engine_cs *engine)
> +{
> +	u32 *a, *b;
> +	int i, err;
> +
> +	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
> +	if (IS_ERR(a))
> +		return PTR_ERR(a);
> +
> +	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
> +	if (IS_ERR(b)) {
> +		err = PTR_ERR(b);
> +		goto err_a;
> +	}
> +
> +	err = 0;
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		if (a[i] != b[i] &&
> +		    !pardon_reg(engine->i915, engine->whitelist.list[i].reg)) {
> +			pr_err("[%d] Whitelisted register 0x%4x not context saved: A=%08x, B=%08x\n",
> +			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg),
> +			       a[i], b[i]);
> +			err = -EINVAL;
> +		}
> +	}
> +
> +	i915_gem_object_unpin_map(B->obj);
> +err_a:
> +	i915_gem_object_unpin_map(A->obj);
> +	return err;
> +}
> +
> +static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
> +{
> +	static const struct {
> +		i915_reg_t reg;
> +		unsigned long gen_mask;
> +	} wo[] = {
> +		{ GEN9_SLICE_COMMON_ECO_CHICKEN1, INTEL_GEN_MASK(9, 9) },
> +	};

What is the difference between pardoned and whitelisted?

> +	u32 offset = i915_mmio_reg_offset(reg);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wo); i++) {
> +		if (INTEL_INFO(i915)->gen_mask & wo[i].gen_mask &&
> +		    i915_mmio_reg_offset(wo[i].reg) == offset)
> +			return true;
> +	}
> +
> +	return false;

You could consolidate into one find_reg which takes the array.

> +}
> +
> +static int neq_whitelisted_registers(struct i915_vma *A,
> +				     struct i915_vma *B,
> +				     struct intel_engine_cs *engine)
> +{
> +	u32 *a, *b;
> +	int i, err;
> +
> +	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
> +	if (IS_ERR(a))
> +		return PTR_ERR(a);
> +
> +	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
> +	if (IS_ERR(b)) {
> +		err = PTR_ERR(b);
> +		goto err_a;
> +	}
> +
> +	err = 0;
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		if (a[i] == b[i] &&
> +		    !writeonly_reg(engine->i915, engine->whitelist.list[i].reg)) {
> +			pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
> +			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
> +			err = -EINVAL;
> +		}
> +	}
> +
> +	i915_gem_object_unpin_map(B->obj);
> +err_a:
> +	i915_gem_object_unpin_map(A->obj);
> +	return err;
> +}

eq and neq could also be cheaply consolidated into one taking table and 
error message.

> +
> +static int live_isolated_whitelist(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct {
> +		struct i915_gem_context *ctx;
> +		struct i915_vma *scratch[2];
> +	} client[2] = {};
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	int i, err = 0;
> +
> +	/*
> +	 * Check that a write into a whitelist register works, but
> +	 * invisible to a second context.
> +	 */
> +
> +	if (!intel_engines_has_context_isolation(i915))
> +		return 0;
> +
> +	if (!i915->kernel_context->ppgtt)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(client); i++) {
> +		struct i915_gem_context *c;
> +
> +		c = kernel_context(i915);
> +		if (IS_ERR(c)) {
> +			err = PTR_ERR(c);
> +			goto err;
> +		}
> +
> +		client[i].scratch[0] = create_scratch(&c->ppgtt->vm, 1024);
> +		if (IS_ERR(client[i].scratch[0])) {
> +			err = PTR_ERR(client[i].scratch[0]);
> +			kernel_context_close(c);
> +			goto err;
> +		}
> +
> +		client[i].scratch[1] = create_scratch(&c->ppgtt->vm, 1024);
> +		if (IS_ERR(client[i].scratch[1])) {
> +			err = PTR_ERR(client[i].scratch[1]);
> +			i915_vma_unpin_and_release(&client[i].scratch[0], 0);
> +			kernel_context_close(c);
> +			goto err;
> +		}
> +
> +		client[i].ctx = c;
> +	}
> +
> +	for_each_engine(engine, i915, id) {
> +		if (!engine->whitelist.count)
> +			continue;
> +
> +		/* Read default values */
> +		err = read_whitelisted_registers(client[0].ctx, engine,
> +						 client[0].scratch[0]);
> +		if (err)
> +			goto err;
> +
> +		/* Try to overwrite registers (should only affect ctx0) */
> +		err = scrub_whitelisted_registers(client[0].ctx, engine);
> +		if (err)
> +			goto err;
> +
> +		/* Read values from ctx1, we expect these to be defaults */
> +		err = read_whitelisted_registers(client[1].ctx, engine,
> +						 client[1].scratch[0]);
> +		if (err)
> +			goto err;
> +
> +		/* Verify that both reads return the same default values */
> +		err = eq_whitelisted_registers(client[0].scratch[0],
> +					       client[1].scratch[0],
> +					       engine);
> +		if (err)
> +			goto err;
> +
> +		/* Read back the updated values in ctx0 */
> +		err = read_whitelisted_registers(client[0].ctx, engine,
> +						 client[0].scratch[1]);
> +		if (err)
> +			goto err;
> +
> +		/* User should be granted privilege to overwhite regs */
> +		err = neq_whitelisted_registers(client[0].scratch[0],
> +						client[0].scratch[1],
> +						engine);
> +		if (err)
> +			goto err;
> +	}
> +
> +err:
> +	for (i = 0; i < ARRAY_SIZE(client); i++) {
> +		if (!client[i].ctx)
> +			break;
> +
> +		i915_vma_unpin_and_release(&client[i].scratch[1], 0);
> +		i915_vma_unpin_and_release(&client[i].scratch[0], 0);
> +		kernel_context_close(client[i].ctx);
> +	}
> +
> +	if (igt_flush_test(i915, I915_WAIT_LOCKED))
> +		err = -EIO;
> +
> +	return err;
> +}
> +
>   static bool verify_gt_engine_wa(struct drm_i915_private *i915,
>   				struct wa_lists *lists, const char *str)
>   {
> @@ -844,6 +1163,7 @@ int intel_workarounds_live_selftests(struct drm_i915_private *i915)
>   	static const struct i915_subtest tests[] = {
>   		SUBTEST(live_dirty_whitelist),
>   		SUBTEST(live_reset_whitelist),
> +		SUBTEST(live_isolated_whitelist),
>   		SUBTEST(live_gpu_reset_gt_engine_workarounds),
>   		SUBTEST(live_engine_reset_gt_engine_workarounds),
>   	};
> 

Regards,

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