Re: [Intel-gfx] [PATCH v2 11/16] drm/i915/pxp: interface for creation of protected contexts

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 3/3/2021 3:16 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:55)
>> Usage of protected objects, coming in a follow-up patch, will be
>> restricted to protected contexts. Contexts can only be marked as
>> protected at creation time and they must be both bannable and not
>> recoverable.
>>
>> When a PXP teardown occurs, all gem contexts marked as protected that
>> have been used at least once will be marked as invalid and all new
>> submissions using them will be rejected. All intel contexts within the
>> invalidated gem contexts will be marked banned.
>> A new flag has been added to the RESET_STATS ioctl to report the
>> invalidation to userspace.
>>
>> v2: split to its own patch and improve doc (Chris), invalidate contexts
>> on teardown
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 59 ++++++++++++++++++-
>>   drivers/gpu/drm/i915/gem/i915_gem_context.h   | 18 ++++++
>>   .../gpu/drm/i915/gem/i915_gem_context_types.h |  2 +
>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 13 ++++
>>   drivers/gpu/drm/i915/pxp/intel_pxp.c          | 38 ++++++++++++
>>   drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
>>   drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  3 +
>>   include/uapi/drm/i915_drm.h                   | 19 ++++++
>>   8 files changed, 150 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> index ca37d93ef5e7..19ac24a3c42c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> @@ -76,6 +76,8 @@
>>   #include "gt/intel_gpu_commands.h"
>>   #include "gt/intel_ring.h"
>>   
>> +#include "pxp/intel_pxp.h"
>> +
>>   #include "i915_drm_client.h"
>>   #include "i915_gem_context.h"
>>   #include "i915_globals.h"
>> @@ -2006,6 +2008,40 @@ static int set_priority(struct i915_gem_context *ctx,
>>          return 0;
>>   }
>>   
>> +static int set_protected(struct i915_gem_context *ctx,
>> +                        const struct drm_i915_gem_context_param *args)
>> +{
>> +       int ret = 0;
>> +
>> +       if (!intel_pxp_is_enabled(&ctx->i915->gt.pxp))
>> +               ret = -ENODEV;
>> +       else if (ctx->client) /* can't change this after creation! */
>> +               ret = -EEXIST;
>> +       else if (args->size)
>> +               ret = -EINVAL;
>> +       else if (!args->value)
>> +               clear_bit(UCONTEXT_PROTECTED, &ctx->user_flags);
>> +       else if (i915_gem_context_is_recoverable(ctx) ||
>> +                !i915_gem_context_is_bannable(ctx))
>> +               ret = -EPERM;
>> +       else
>> +               set_bit(UCONTEXT_PROTECTED, &ctx->user_flags);
>> +
>> +       return ret;
>> +}
>> +
>> +static int get_protected(struct i915_gem_context *ctx,
>> +                        struct drm_i915_gem_context_param *args)
>> +{
>> +       if (!intel_pxp_is_enabled(&ctx->i915->gt.pxp))
>> +               return -ENODEV;
>> +
>> +       args->size = 0;
>> +       args->value = i915_gem_context_can_use_protected_content(ctx);
>> +
>> +       return 0;
>> +}
>> +
>>   static int ctx_setparam(struct drm_i915_file_private *fpriv,
>>                          struct i915_gem_context *ctx,
>>                          struct drm_i915_gem_context_param *args)
>> @@ -2038,6 +2074,8 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
>>                          ret = -EPERM;
>>                  else if (args->value)
>>                          i915_gem_context_set_bannable(ctx);
>> +               else if (i915_gem_context_can_use_protected_content(ctx))
>> +                       ret = -EPERM; /* can't clear this for protected contexts */
>>                  else
>>                          i915_gem_context_clear_bannable(ctx);
>>                  break;
>> @@ -2045,10 +2083,12 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
>>          case I915_CONTEXT_PARAM_RECOVERABLE:
>>                  if (args->size)
>>                          ret = -EINVAL;
>> -               else if (args->value)
>> -                       i915_gem_context_set_recoverable(ctx);
>> -               else
>> +               else if (!args->value)
>>                          i915_gem_context_clear_recoverable(ctx);
>> +               else if (i915_gem_context_can_use_protected_content(ctx))
>> +                       ret = -EPERM; /* can't set this for protected contexts */
>> +               else
>> +                       i915_gem_context_set_recoverable(ctx);
>>                  break;
>>   
>>          case I915_CONTEXT_PARAM_PRIORITY:
>> @@ -2075,6 +2115,10 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
>>                  ret = set_ringsize(ctx, args);
>>                  break;
>>   
>> +       case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
>> +               ret = set_protected(ctx, args);
>> +               break;
>> +
>>          case I915_CONTEXT_PARAM_BAN_PERIOD:
>>          default:
>>                  ret = -EINVAL;
>> @@ -2532,6 +2576,10 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>>                  ret = get_ringsize(ctx, args);
>>                  break;
>>   
>> +       case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
>> +               ret = get_protected(ctx, args);
>> +               break;
>> +
>>          case I915_CONTEXT_PARAM_BAN_PERIOD:
>>          default:
>>                  ret = -EINVAL;
>> @@ -2592,6 +2640,11 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>>          args->batch_active = atomic_read(&ctx->guilty_count);
>>          args->batch_pending = atomic_read(&ctx->active_count);
>>   
>> +       /* re-use args->flags for output flags */
>> +       args->flags = 0;
>> +       if (i915_gem_context_invalidated(ctx))
>> +               args->flags |= I915_CONTEXT_INVALIDATED;
>> +
>>          ret = 0;
>>   out:
>>          rcu_read_unlock();
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> index b5c908f3f4f2..b04d4eeb0500 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
>> @@ -108,6 +108,24 @@ i915_gem_context_clear_user_engines(struct i915_gem_context *ctx)
>>          clear_bit(CONTEXT_USER_ENGINES, &ctx->flags);
>>   }
>>   
>> +static inline bool
>> +i915_gem_context_invalidated(const struct i915_gem_context *ctx)
>> +{
>> +       return test_bit(CONTEXT_INVALID, &ctx->flags);
>> +}
>> +
>> +static inline void
>> +i915_gem_context_set_invalid(struct i915_gem_context *ctx)
>> +{
>> +       set_bit(CONTEXT_INVALID, &ctx->flags);
>> +}
>> +
>> +static inline bool
>> +i915_gem_context_can_use_protected_content(const struct i915_gem_context *ctx)
>> +{
>> +       return test_bit(UCONTEXT_PROTECTED, &ctx->user_flags);
>> +}
>> +
>>   /* i915_gem_context.c */
>>   void i915_gem_init__contexts(struct drm_i915_private *i915);
>>   
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> index d5bc75508048..79a87268b8da 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
>> @@ -130,6 +130,7 @@ struct i915_gem_context {
>>   #define UCONTEXT_BANNABLE              2
>>   #define UCONTEXT_RECOVERABLE           3
>>   #define UCONTEXT_PERSISTENCE           4
>> +#define UCONTEXT_PROTECTED             5
>>   
>>          /**
>>           * @flags: small set of booleans
>> @@ -137,6 +138,7 @@ struct i915_gem_context {
>>          unsigned long flags;
>>   #define CONTEXT_CLOSED                 0
>>   #define CONTEXT_USER_ENGINES           1
>> +#define CONTEXT_INVALID                        2
>>   
>>          struct mutex mutex;
>>   
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index fe170186dd42..e503c9f789c0 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -21,6 +21,8 @@
>>   #include "gt/intel_gt_pm.h"
>>   #include "gt/intel_ring.h"
>>   
>> +#include "pxp/intel_pxp.h"
>> +
>>   #include "i915_drv.h"
>>   #include "i915_gem_clflush.h"
>>   #include "i915_gem_context.h"
>> @@ -726,6 +728,11 @@ static int eb_select_context(struct i915_execbuffer *eb)
>>          if (unlikely(!ctx))
>>                  return -ENOENT;
>>   
>> +       if (i915_gem_context_invalidated(ctx)) {
>> +               i915_gem_context_put(ctx);
>> +               return -EIO;
>> +       }
>> +
>>          eb->gem_context = ctx;
>>          if (rcu_access_pointer(ctx->vm))
>>                  eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
>> @@ -2761,6 +2768,12 @@ eb_select_engine(struct i915_execbuffer *eb)
>>   
>>          intel_gt_pm_get(ce->engine->gt);
>>   
>> +       if (i915_gem_context_can_use_protected_content(eb->gem_context)) {
>> +               err = intel_pxp_wait_for_termination_completion(&ce->engine->gt->pxp);
>> +               if (err)
>> +                       goto err;
> This should check for context_invalidated
>
>> +       }
>> +
>>          if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>>                  err = intel_context_alloc_state(ce);
>>                  if (err)
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> index 0ca1c2c16972..5912e4a12d94 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> @@ -6,6 +6,7 @@
>>   #include "intel_pxp.h"
>>   #include "intel_pxp_irq.h"
>>   #include "intel_pxp_tee.h"
>> +#include "gem/i915_gem_context.h"
>>   #include "gt/intel_context.h"
>>   #include "i915_drv.h"
>>   
>> @@ -135,3 +136,40 @@ int intel_pxp_wait_for_termination_completion(struct intel_pxp *pxp)
>>          return ret;
>>   }
>>   
>> +void intel_pxp_invalidate(struct intel_pxp *pxp)
>> +{
>> +       struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
>> +       struct i915_gem_context *ctx, *cn;
>> +
>> +       /* ban all contexts marked as protected */
>> +       spin_lock_irq(&i915->gem.contexts.lock);
>> +       list_for_each_entry_safe(ctx, cn, &i915->gem.contexts.list, link) {
>> +               struct i915_gem_engines_iter it;
>> +               struct intel_context *ce;
>> +
>> +               if (!kref_get_unless_zero(&ctx->ref))
>> +                       continue;
>> +
>> +               if (likely(!i915_gem_context_can_use_protected_content(ctx)))
>> +                       continue;
>> +
>> +               if (i915_gem_context_invalidated(ctx))
>> +                       continue;
>> +
>> +               spin_unlock_irq(&i915->gem.contexts.lock);
>> +
>> +               for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>> +                       if (test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>> +                               intel_context_set_banned(ce);
> That's a pair of unserialised operations. However, you can just set
> banned before we even try to alloc. But at the moment, it's very

I wanted to avoid banning the intel_contexts if none of them had been 
allocated (i.e. gem_context created but not used).

> spurious to call intel_context_set_banned() as no action is performed to
> flush the ban to the backend, and execbuf is primarily checking the GEM
> context invalid bit. It looks a bit flimsy...
>
> The impact of this loop would be to cancel all pending requests (but not
> the inflight request), sounds like you should just call revoke_context
> to remove the inflight context and pending requests from the HW.

I agree it is flimsy, but the whole process is best effort so I didn't 
want to over-complicate it by taking out the inflight request. The 
moment we get the interrupt the keys are already invalid so it's most 
likely already too late to save the inflight request if it is using one 
of the sessions. I've tried to prioritize getting the session back up as 
soon as possible.

What's revoke_context? I can't find it.

>
>> +                               i915_gem_context_set_invalid(ctx);
>> +                       }
>> +               }
>> +               i915_gem_context_unlock_engines(ctx);
>> +
>> +               spin_lock_irq(&i915->gem.contexts.lock);
>> +               list_safe_reset_next(ctx, cn, link);
>> +               i915_gem_context_put(ctx);
>> +       }
>> +       spin_unlock_irq(&i915->gem.contexts.lock);
> And then there's a race condition where we can create a new protect
> context, submit an execbuf using the global session between the call to
> intel_pxp_invalidate and terminate_session_and_global. That's covered by
> irq_handler calling mark_termination_in_progress and execbuf waiting
> for that termination to complete. Almost.

Why almost? what am I missing?

>
> This will result in userspace context lost upon runtime suspend? That
> can be quite catastrophic for the system...

Unfortunately S0ix kills the keys, so yes, unless we decide that we 
don't want to ban protected contexts when the keys are invalidated. That 
was a purely software design decision, the contexts are in no way tied 
to PXP in HW.

>
>> +}
>> +
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> index 89cf66c9bef3..e36200833095 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> @@ -38,6 +38,7 @@ void intel_pxp_init(struct intel_pxp *pxp);
>>   void intel_pxp_fini(struct intel_pxp *pxp);
>>   
>>   int intel_pxp_wait_for_termination_completion(struct intel_pxp *pxp);
>> +void intel_pxp_invalidate(struct intel_pxp *pxp);
>>   #else
>>   static inline void intel_pxp_init(struct intel_pxp *pxp)
>>   {
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
>> index bb981d38c2fe..527217b3db23 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
>> @@ -111,6 +111,9 @@ int intel_pxp_arb_terminate_session_with_global_terminate(struct intel_pxp *pxp)
>>   
>>          lockdep_assert_held(&pxp->mutex);
>>   
>> +       /* invalidate protected objects */
>> +       intel_pxp_invalidate(pxp);
>> +
>>          /* terminate the hw sessions */
>>          ret = intel_pxp_submit_session_termination(pxp, ARB_SESSION);
>>          if (ret) {
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 56c6bfe6c2d0..0f5456046c4c 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -1694,6 +1694,24 @@ struct drm_i915_gem_context_param {
>>    * Default is 16 KiB.
>>    */
>>   #define I915_CONTEXT_PARAM_RINGSIZE    0xc
>> +
>> +/*
>> + * I915_CONTEXT_PARAM_PROTECTED_CONTENT:
>> + *
>> + * Enable usage of protected content with the context. This flag can only be
>> + * set at context creation time and, when set to true, must be preceded by
>> + * an explicit setting of I915_CONTEXT_PARAM_RECOVERABLE to false. This flag
>> + * can't be set to true in conjunction with setting the
>> + * I915_CONTEXT_PARAM_BANNABLE flag to false.
> I haven't seen any interaction with bannable here. Since you just ignore
> the bannable flag.

But I still want to stop userspace from setting the context as not 
bannable. Once I know that protected implies bannable I don't need to 
check the latter.

>
>> + *
>> + * Given the numerous restriction on this flag, there are several unique
>> + * failure cases:
>> + *
>> + * -ENODEV: feature not available
>> + * -EEXIST: trying to modify an existing context
>> + * -EPERM: trying to mark a recoverable or not bannable context as protected
> Also -EIO from execbuf. Do we want to pick EACCES?

instead of EIO? sounds reasonable.

>
>> + */
>> +#define I915_CONTEXT_PARAM_PROTECTED_CONTENT    0xd
>>   /* Must be kept compact -- no holes and well documented */
>>   
>>          __u64 value;
>> @@ -1924,6 +1942,7 @@ struct drm_i915_reg_read {
>>   struct drm_i915_reset_stats {
>>          __u32 ctx_id;
>>          __u32 flags;
>> +#define I915_CONTEXT_INVALIDATED 0x1
> Mention the side effect for protected content contexts.

ok.

>
>>   
>>          /* All resets since boot/module reload, for all contexts */
>>          __u32 reset_count;
>> -- 
>> 2.29.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>

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