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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2019-04-16 15:50:27)
> 
> 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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.

copy-and-paste.

> > +     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?

Because we are playing with HW registers, I want reset to work safely.
So long as we have init_breadcrumb, the payload scrubbing should work.
Ok.

> > +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?

Pardoned is for non-context registers that are already exposed as
RING_NONPRIV ABI. Mistakes already made.

writeonly is for a register that doesn't seem to stick but everyone
believes is a crucial w/a for glk.

> > +     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.

Next up, you'll be saying we could bsearch by addr! :)

> > +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.

And op. The preamble is the same, the loop innards are not, but we can
just make a callback.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux