[v4,14/17] drm/i915/pxp: User interface for Protected buffer

Commit Message

Daniele Ceraolo Spurio May 25, 2021, 5:48 a.m. UTC

From: Bommu Krishnaiah <[e-mail]>

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.

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 <[e-mail]>
Signed-off-by: Daniele Ceraolo Spurio <[e-mail]>
Cc: Telukuntla Sreedhar <[e-mail]>
Cc: Kondapally Kalyan <[e-mail]>
Cc: Gupta Anshuman <[e-mail]>
Cc: Huang Sean Z <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Lionel Landwerlin <[e-mail]>
Cc: Jason Ekstrand <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
---
 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(-)

Comments

Daniel Vetter May 25, 2021, 1:32 p.m. UTC | #1

On Mon, May 24, 2021 at 10:48:00PM -0700, Daniele Ceraolo Spurio wrote:
> From: Bommu Krishnaiah <[e-mail]>
> 
> 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.
> 
> 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 <[e-mail]>
> Signed-off-by: Daniele Ceraolo Spurio <[e-mail]>
> Cc: Telukuntla Sreedhar <[e-mail]>
> Cc: Kondapally Kalyan <[e-mail]>
> Cc: Gupta Anshuman <[e-mail]>
> Cc: Huang Sean Z <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> ---
>  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

For easier review it would be nice to have all the uapi and its kerneldoc
in one patch. I feel like I've missed parts.

> @@ -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

Not finding the kerneldoc for this. Also is this a CTX_CREATE_EXT flag, so
guaranteed to be immutable, or is this another CTX_SETPARAM flag, which
would be mutable and we really want to get away from that paradigm?

> + * at submission time on the validity of the objects involved, which can lead to
> + * the following errors being returned from the execbuf ioctl:

I think this also should be put into the execbuf ioctl structs. Plus I
think RESET_STATS should also explain how this interacts and all that.
Here we should just reference this other sections imo.

Yes this means a bit more to type because not even the bare bones of these
other ioctl kerneldocs exists, but we have to start somewhere.

Cheers, Daniel

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

Tang, CQ May 25, 2021, 6:36 p.m. UTC | #2

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of
> Daniele Ceraolo Spurio
> Sent: Monday, May 24, 2021 10:48 PM
> To: [e-mail]
> Cc: Vetter, Daniel <[e-mail]>; Huang Sean Z
> <[e-mail]>; [e-mail]; Chris Wilson
> <[e-mail]>; Kondapally Kalyan
> <[e-mail]>; Bommu, Krishnaiah
> <[e-mail]>
> Subject: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for
> Protected buffer
> 
> From: Bommu Krishnaiah <[e-mail]>
> 
> 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?

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?

--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 <[e-mail]>
> Signed-off-by: Daniele Ceraolo Spurio <[e-mail]>
> Cc: Telukuntla Sreedhar <[e-mail]>
> Cc: Kondapally Kalyan <[e-mail]>
> Cc: Gupta Anshuman <[e-mail]>
> Cc: Huang Sean Z <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> Cc: Jason Ekstrand <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> ---
>  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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniele Ceraolo Spurio May 27, 2021, 2:03 a.m. UTC | #3

On 5/25/2021 6:32 AM, Daniel Vetter wrote:
> On Mon, May 24, 2021 at 10:48:00PM -0700, Daniele Ceraolo Spurio wrote:
>> From: Bommu Krishnaiah <[e-mail]>
>>
>> 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.
>>
>> 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 <[e-mail]>
>> Signed-off-by: Daniele Ceraolo Spurio <[e-mail]>
>> Cc: Telukuntla Sreedhar <[e-mail]>
>> Cc: Kondapally Kalyan <[e-mail]>
>> Cc: Gupta Anshuman <[e-mail]>
>> Cc: Huang Sean Z <[e-mail]>
>> Cc: Chris Wilson <[e-mail]>
>> Cc: Lionel Landwerlin <[e-mail]>
>> Cc: Jason Ekstrand <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> ---
>>   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
> For easier review it would be nice to have all the uapi and its kerneldoc
> in one patch. I feel like I've missed parts.

The previous part (protected context) in patch 11, I'll just squash them.

>
>> @@ -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
> Not finding the kerneldoc for this. Also is this a CTX_CREATE_EXT flag, so
> guaranteed to be immutable, or is this another CTX_SETPARAM flag, which
> would be mutable and we really want to get away from that paradigm?

it's immutable. We definitely don't want users toggling protection at 
runtime.

>
>> + * at submission time on the validity of the objects involved, which can lead to
>> + * the following errors being returned from the execbuf ioctl:
> I think this also should be put into the execbuf ioctl structs. Plus I
> think RESET_STATS should also explain how this interacts and all that.
> Here we should just reference this other sections imo.

sure, will do.

Daniele

>
> Yes this means a bit more to type because not even the bare bones of these
> other ioctl kerneldocs exists, but we have to start somewhere.
>
> Cheers, Daniel
>
>> + *
>> + * -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
>>

Daniele Ceraolo Spurio May 27, 2021, 2:13 a.m. UTC | #4

On 5/25/2021 11:36 AM, Tang, CQ wrote:
>
>> -----Original Message-----
>> From: Intel-gfx <[e-mail]> On Behalf Of
>> Daniele Ceraolo Spurio
>> Sent: Monday, May 24, 2021 10:48 PM
>> To: [e-mail]
>> Cc: Vetter, Daniel <[e-mail]>; Huang Sean Z
>> <[e-mail]>; [e-mail]; Chris Wilson
>> <[e-mail]>; Kondapally Kalyan
>> <[e-mail]>; Bommu, Krishnaiah
>> <[e-mail]>
>> Subject: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for
>> Protected buffer
>>
>> From: Bommu Krishnaiah <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Daniele Ceraolo Spurio <[e-mail]>
>> Cc: Telukuntla Sreedhar <[e-mail]>
>> Cc: Kondapally Kalyan <[e-mail]>
>> Cc: Gupta Anshuman <[e-mail]>
>> Cc: Huang Sean Z <[e-mail]>
>> Cc: Chris Wilson <[e-mail]>
>> Cc: Lionel Landwerlin <[e-mail]>
>> Cc: Jason Ekstrand <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> ---
>>   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
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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