Re: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for Protected buffer

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: "Tang, CQ" <cq.tang@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: Kondapally Kalyan <kalyan.kondapally@intel.com>,
	Huang Sean Z <sean.z.huang@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Chris Wilson <chris@chris-wilson.co.uk>,
	"Vetter, Daniel" <daniel.vetter@intel.com>,
	"Bommu, Krishnaiah" <krishnaiah.bommu@intel.com>
Subject: Re: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for Protected buffer
Date: Wed, 26 May 2021 19:13:46 -0700	[thread overview]
Message-ID: <d231efee-4108-d027-86af-1908d01265ac@intel.com> (raw)
In-Reply-To: <728fa3c9e484402d82fb7fb124496bdb@intel.com>

On 5/25/2021 11:36 AM, Tang, CQ wrote:
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Daniele Ceraolo Spurio
>> Sent: Monday, May 24, 2021 10:48 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Vetter, Daniel <daniel.vetter@intel.com>; Huang Sean Z
>> <sean.z.huang@intel.com>; dri-devel@lists.freedesktop.org; Chris Wilson
>> <chris@chris-wilson.co.uk>; Kondapally Kalyan
>> <kalyan.kondapally@intel.com>; Bommu, Krishnaiah
>> <krishnaiah.bommu@intel.com>
>> Subject: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for
>> Protected buffer
>>
>> From: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
>>
>> This api allow user mode to create Protected buffers. Only contexts marked
>> as protected are allowed to operate on protected buffers.
>>
>> We only allow setting the flags at creation time.
>>
>> All protected objects that have backing storage will be considered invalid
>> when the session is destroyed and they won't be usable anymore.
> Then these protected objects will be hanging in the system till user call gem_close() to free them?
> If the objects won't be usable anymore, why don't we automatically free these objects when the session is destroyed?

Auto-freeing an object would require some extra reworks (i.e. plumbing 
PXP status checks in a lot of places), so to keep things simple, the 
protected objects have the same lifetime as normal ones. A user can keep 
non protected objects hanging around as long as they want, it's not like 
the protected ones are worse in that sense.

> How is a session started/destroyed?  From the code, intel_pxp_init() is called when loading i915 driver, so I think session lifetime is the same as i915 driver lifetime.
> Can we start multiple sessions after loading the driver?

A session is started with a call into the PXP mei device and can be 
manually destroyed with a specific instruction submitted via a video 
engine, but the HW also invalidates the keys when certain events occurs 
(e.g. suspend/resume). The HW supports multiple sessions, but we 
currently only use one in i915; it is automatically started when a 
protected context is submitted and then kept running until an 
invalidation event occurs. See patch 12 for details.

Daniele

>
> --CQ
>
>> Given that the PXP HW supports multiple modes (but we currently only care
>> about one), a flag variable has been reserved in the structure used in the
>> create_ext ioctl for possible future updates.
>>
>> This is a rework of the original code by Bommu Krishnaiah. I've kept
>> authorship unchanged since significant chunks have not been modified.
>>
>> v2: split context changes, fix defines and improve documentation (Chris),
>>      add object invalidation logic
>> v3: fix spinlock definition and usage, only validate objects when
>>      they're first added to a context lut, only remove them once (Chris),
>>      make protected context flag not mandatory in protected object execbuf
>>      to avoid abuse (Lionel)
>> v4: rebase to new gem_create_ext
>>
>> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Telukuntla Sreedhar <sreedhar.telukuntla@intel.com>
>> Cc: Kondapally Kalyan <kalyan.kondapally@intel.com>
>> Cc: Gupta Anshuman <Anshuman.Gupta@intel.com>
>> Cc: Huang Sean Z <sean.z.huang@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_create.c    | 26 ++++++++++++
>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 15 +++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c    |  6 +++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h    | 12 ++++++
>>   .../gpu/drm/i915/gem/i915_gem_object_types.h  | 13 ++++++
>>   drivers/gpu/drm/i915/pxp/intel_pxp.c          | 41 +++++++++++++++++++
>>   drivers/gpu/drm/i915/pxp/intel_pxp.h          | 13 ++++++
>>   drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |  5 +++
>>   include/uapi/drm/i915_drm.h                   | 33 ++++++++++++++-
>>   9 files changed, 163 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> index 548ddf39d853..c14be3882c35 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> @@ -6,6 +6,7 @@
>>   #include "gem/i915_gem_ioctls.h"
>>   #include "gem/i915_gem_lmem.h"
>>   #include "gem/i915_gem_region.h"
>> +#include "pxp/intel_pxp.h"
>>
>>   #include "i915_drv.h"
>>   #include "i915_trace.h"
>> @@ -99,7 +100,11 @@ i915_gem_setup(struct drm_i915_gem_object *obj,
>> u64 size)
>>
>>   	GEM_BUG_ON(size != obj->base.size);
>>
>> +	if (obj->user_flags & I915_GEM_OBJECT_PROTECTED)
>> +		intel_pxp_object_add(obj);
>> +
>>   	trace_i915_gem_object_create(obj);
>> +
>>   	return 0;
>>   }
>>
>> @@ -344,8 +349,29 @@ static int ext_set_placements(struct
>> i915_user_extension __user *base,
>>   	return set_placements(&ext, data);
>>   }
>>
>> +static int ext_set_protected(struct i915_user_extension __user *base,
>> +void *data) {
>> +	struct drm_i915_gem_create_ext_protected_content ext;
>> +	struct create_ext *ext_data = data;
>> +
>> +	if (copy_from_user(&ext, base, sizeof(ext)))
>> +		return -EFAULT;
>> +
>> +	if (ext.flags)
>> +		return -EINVAL;
>> +
>> +	if (!intel_pxp_is_enabled(&ext_data->i915->gt.pxp))
>> +		return -ENODEV;
>> +
>> +	ext_data->vanilla_object->user_flags |=
>> I915_GEM_OBJECT_PROTECTED;
>> +
>> +	return 0;
>> +}
>> +
>> +
>>   static const i915_user_extension_fn create_extensions[] = {
>>   	[I915_GEM_CREATE_EXT_MEMORY_REGIONS] =
>> ext_set_placements,
>> +	[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] =
>> ext_set_protected,
>>   };
>>
>>   /**
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index c08e28847064..5dd813d04a9f 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -839,6 +839,21 @@ static struct i915_vma *eb_lookup_vma(struct
>> i915_execbuffer *eb, u32 handle)
>>   		if (unlikely(!obj))
>>   			return ERR_PTR(-ENOENT);
>>
>> +		/*
>> +		 * If the user has opted-in for protected-object tracking,
>> make
>> +		 * sure the object encryption can be used.
>> +		 * We only need to do this when the object is first used with
>> +		 * this context, because the context itself will be banned
>> when
>> +		 * the protected objects become invalid.
>> +		 */
>> +		if (i915_gem_context_uses_protected_content(eb-
>>> gem_context) &&
>> +		    i915_gem_object_is_protected(obj)) {
>> +			if (!intel_pxp_is_active(&vm->gt->pxp))
>> +				return ERR_PTR(-ENODEV);
>> +			if (!i915_gem_object_has_valid_protection(obj))
>> +				return ERR_PTR(-ENOEXEC);
>> +		}
>> +
>>   		vma = i915_vma_instance(obj, vm, NULL);
>>   		if (IS_ERR(vma)) {
>>   			i915_gem_object_put(obj);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 28144410df86..b47fa0a7b25a 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/sched/mm.h>
>>
>>   #include "display/intel_frontbuffer.h"
>> +#include "pxp/intel_pxp.h"
>>   #include "i915_drv.h"
>>   #include "i915_gem_clflush.h"
>>   #include "i915_gem_context.h"
>> @@ -70,6 +71,8 @@ void i915_gem_object_init(struct
>> drm_i915_gem_object *obj,
>>   	INIT_LIST_HEAD(&obj->lut_list);
>>   	spin_lock_init(&obj->lut_lock);
>>
>> +	INIT_LIST_HEAD(&obj->pxp_link);
>> +
>>   	spin_lock_init(&obj->mmo.lock);
>>   	obj->mmo.offsets = RB_ROOT;
>>
>> @@ -232,6 +235,9 @@ static void __i915_gem_free_objects(struct
>> drm_i915_private *i915,
>>   			spin_unlock(&obj->vma.lock);
>>   		}
>>
>> +		if (i915_gem_object_has_valid_protection(obj))
>> +			intel_pxp_object_remove(obj);
>> +
>>   		__i915_gem_object_free_mmaps(obj);
>>
>>   		GEM_BUG_ON(!list_empty(&obj->lut_list));
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index 2ebd79537aea..61b101560352 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -288,6 +288,18 @@ i915_gem_object_never_mmap(const struct
>> drm_i915_gem_object *obj)
>>   	return i915_gem_object_type_has(obj,
>> I915_GEM_OBJECT_NO_MMAP);  }
>>
>> +static inline bool
>> +i915_gem_object_is_protected(const struct drm_i915_gem_object *obj) {
>> +	return obj->user_flags & I915_GEM_OBJECT_PROTECTED; }
>> +
>> +static inline bool
>> +i915_gem_object_has_valid_protection(const struct drm_i915_gem_object
>> +*obj) {
>> +	return i915_gem_object_is_protected(obj) &&
>> +!list_empty(&obj->pxp_link); }
>> +
>>   static inline bool
>>   i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
>> { diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> index 0727d0c76aa0..a698ad0ef7f6 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -167,6 +167,11 @@ struct drm_i915_gem_object {
>>   	} mmo;
>>
>>   	I915_SELFTEST_DECLARE(struct list_head st_link);
>> +	/**
>> +	 * @user_flags: small set of booleans set by the user
>> +	 */
>> +	unsigned long user_flags;
>> +#define I915_GEM_OBJECT_PROTECTED BIT(0)
>>
>>   	unsigned long flags;
>>   #define I915_BO_ALLOC_CONTIGUOUS BIT(0) @@ -294,6 +299,14 @@
>> struct drm_i915_gem_object {
>>   		bool dirty:1;
>>   	} mm;
>>
>> +	/*
>> +	 * When the PXP session is invalidated, we need to mark all
>> protected
>> +	 * objects as invalid. To easily do so we add them all to a list. The
>> +	 * presence on the list is used to check if the encryption is valid or
>> +	 * not.
>> +	 */
>> +	struct list_head pxp_link;
>> +
>>   	/** Record of address bit 17 of each page at last unbind. */
>>   	unsigned long *bit_17;
>>
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> index 2291c68fd3a0..e6a59eb05eae 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> @@ -69,6 +69,9 @@ void intel_pxp_init(struct intel_pxp *pxp)
>>   	if (!HAS_PXP(gt->i915))
>>   		return;
>>
>> +	spin_lock_init(&pxp->lock);
>> +	INIT_LIST_HEAD(&pxp->protected_objects);
>> +
>>   	/*
>>   	 * we'll use the completion to check if there is a termination pending,
>>   	 * so we start it as completed and we reinit it when a termination
>> @@ -177,11 +180,49 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp)
>>   	intel_pxp_irq_disable(pxp);
>>   }
>>
>> +int intel_pxp_object_add(struct drm_i915_gem_object *obj) {
>> +	struct intel_pxp *pxp = &to_i915(obj->base.dev)->gt.pxp;
>> +
>> +	if (!intel_pxp_is_enabled(pxp))
>> +		return -ENODEV;
>> +
>> +	if (!list_empty(&obj->pxp_link))
>> +		return -EEXIST;
>> +
>> +	spin_lock_irq(&pxp->lock);
>> +	list_add(&obj->pxp_link, &pxp->protected_objects);
>> +	spin_unlock_irq(&pxp->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +void intel_pxp_object_remove(struct drm_i915_gem_object *obj) {
>> +	struct intel_pxp *pxp = &to_i915(obj->base.dev)->gt.pxp;
>> +
>> +	if (!intel_pxp_is_enabled(pxp))
>> +		return;
>> +
>> +	spin_lock_irq(&pxp->lock);
>> +	list_del_init(&obj->pxp_link);
>> +	spin_unlock_irq(&pxp->lock);
>> +}
>> +
>>   void intel_pxp_invalidate(struct intel_pxp *pxp)  {
>>   	struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
>> +	struct drm_i915_gem_object *obj, *tmp;
>>   	struct i915_gem_context *ctx, *cn;
>>
>> +	/* delete objects that have been used with the invalidated session
>> */
>> +	spin_lock_irq(&pxp->lock);
>> +	list_for_each_entry_safe(obj, tmp, &pxp->protected_objects,
>> pxp_link) {
>> +		if (i915_gem_object_has_pages(obj))
>> +			list_del_init(&obj->pxp_link);
>> +	}
>> +	spin_unlock_irq(&pxp->lock);
>> +
>>   	/* 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)
>> { diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> index 1f9871e64096..3500d7896058 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> @@ -9,6 +9,8 @@
>>   #include "gt/intel_gt_types.h"
>>   #include "intel_pxp_types.h"
>>
>> +struct drm_i915_gem_object;
>> +
>>   static inline struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)  {
>>   	return container_of(pxp, struct intel_gt, pxp); @@ -33,6 +35,9 @@
>> void intel_pxp_fini_hw(struct intel_pxp *pxp);
>>
>>   void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);  int
>> intel_pxp_start(struct intel_pxp *pxp);
>> +
>> +int intel_pxp_object_add(struct drm_i915_gem_object *obj); void
>> +intel_pxp_object_remove(struct drm_i915_gem_object *obj);
>>   void intel_pxp_invalidate(struct intel_pxp *pxp);  #else  static inline void
>> intel_pxp_init(struct intel_pxp *pxp) @@ -47,6 +52,14 @@ static inline int
>> intel_pxp_start(struct intel_pxp *pxp)  {
>>   	return 0;
>>   }
>> +
>> +static inline int intel_pxp_object_add(struct drm_i915_gem_object *obj)
>> +{
>> +	return 0;
>> +}
>> +static inline void intel_pxp_object_remove(struct drm_i915_gem_object
>> +*obj) { }
>>   #endif
>>
>>   #endif /* __INTEL_PXP_H__ */
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
>> b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
>> index b3ae49dd73a8..cc510416eac6 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
>> @@ -7,7 +7,9 @@
>>   #define __INTEL_PXP_TYPES_H__
>>
>>   #include <linux/completion.h>
>> +#include <linux/list.h>
>>   #include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>>   #include <linux/types.h>
>>   #include <linux/workqueue.h>
>>
>> @@ -43,6 +45,9 @@ struct intel_pxp {
>>   #define PXP_TERMINATION_REQUEST  BIT(0)  #define
>> PXP_TERMINATION_COMPLETE BIT(1)
>>   #define PXP_INVAL_REQUIRED       BIT(2)
>> +
>> +	spinlock_t lock; /* protects the objects list */
>> +	struct list_head protected_objects;
>>   };
>>
>>   #endif /* __INTEL_PXP_TYPES_H__ */
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 3cc33fcbf520..d39b8cb4bfc5 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -1768,7 +1768,7 @@ struct drm_i915_gem_context_param {
>>   	__u64 value;
>>   };
>>
>> -/*
>> +/**
>>    * Context SSEU programming
>>    *
>>    * It may be necessary for either functional or performance reason to
>> configure @@ -2669,8 +2669,12 @@ struct drm_i915_gem_create_ext {
>>   	 *
>>   	 * For I915_GEM_CREATE_EXT_MEMORY_REGIONS usage see
>>   	 * struct drm_i915_gem_create_ext_memory_regions.
>> +	 *
>> +	 * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>> +	 * struct drm_i915_gem_create_ext_protected_content.
>>   	 */
>>   #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>> +#define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
>>   	__u64 extensions;
>>   };
>>
>> @@ -2728,6 +2732,33 @@ struct
>> drm_i915_gem_create_ext_memory_regions {
>>   	__u64 regions;
>>   };
>>
>> +/**
>> + * struct drm_i915_gem_create_ext_protected_content - The
>> + * I915_OBJECT_PARAM_PROTECTED_CONTENT extension.
>> + *
>> + * If this extension is provided, buffer contents are expected to be
>> + * protected by PXP encryption and requires decryption for scan out
>> + * and processing. This is only possible on platforms that have PXP
>> +enabled,
>> + * on all other scenarios ysing this extension will cause the ioctl to
>> +fail
>> + * and return -ENODEV. The flags parameter is reserved for future
>> +expansion and
>> + * must currently be set to zero.
>> + *
>> + * The buffer contents are considered invalid after a PXP session teardown.
>> + *
>> + * The encryption is guaranteed to be processed correctly only if the
>> +object
>> + * is submitted with a context created using the
>> + * I915_CONTEXT_PARAM_PROTECTED_CONTENT flag. This will also enable
>> +extra checks
>> + * at submission time on the validity of the objects involved, which
>> +can lead to
>> + * the following errors being returned from the execbuf ioctl:
>> + *
>> + * -ENODEV: PXP session not currently active
>> + * -ENOEXEC: buffer has become invalid after a teardown event  */
>> +struct drm_i915_gem_create_ext_protected_content {
>> +	struct i915_user_extension base;
>> +	__u32 flags;
>> +};
>> +
>>   /* ID of the protected content session managed by i915 when PXP is active
>> */  #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>>
>> --
>> 2.29.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx