Re: [Intel-gfx] [PATCH v7 05/17] drm/i915/pxp: Implement funcs to create the TEE channel

On 8/31/2021 2:08 PM, Rodrigo Vivi wrote:

On Fri, Aug 27, 2021 at 06:27:26PM -0700, Daniele Ceraolo Spurio wrote:

...

From: "Huang, Sean Z" sean.z.huang@intel.com

Implement the funcs to create the TEE channel, so kernel can send the TEE commands directly to TEE for creating the arbitrary (default) session.

v2: fix locking, don't pollute dev_priv (Chris)

v3: wait for mei PXP component to be bound.

v4: drop the wait, as the component might be bound after i915 load completes. We'll instead check when sending a tee message.

v5: fix an issue with mei_pxp module removal

Signed-off-by: Huang, Sean Z sean.z.huang@intel.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com #v4

---

drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/pxp/intel_pxp.c | 13 ++++ drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 76 ++++++++++++++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_tee.h | 14 ++++ drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 6 ++ 5 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_tee.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 157644ef5886..cc9fe99ca5c5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -282,7 +282,8 @@ i915-y += i915_perf.o

# Protected execution platform (PXP) support i915-$(CONFIG_DRM_I915_PXP) += \

• pxp/intel_pxp.o

• pxp/intel_pxp.o \

• pxp/intel_pxp_tee.o

  # Post-mortem debug and GPU hang state capture i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index 7b2053902146..400deaea2d8a 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -3,6 +3,7 @@

• Copyright(c) 2020 Intel Corporation.

*/ #include "intel_pxp.h" +#include "intel_pxp_tee.h" #include "gt/intel_context.h" #include "i915_drv.h"

@@ -50,7 +51,16 @@ void intel_pxp_init(struct intel_pxp *pxp) if (ret) return;

• ret = intel_pxp_tee_component_init(pxp);
• if (ret)

• goto out_context;
  

• drm_info(&gt->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n");
• return;

+out_context:

• destroy_vcs_context(pxp); }

  void intel_pxp_fini(struct intel_pxp *pxp)

@@ -58,5 +68,8 @@ void intel_pxp_fini(struct intel_pxp *pxp) if (!intel_pxp_is_enabled(pxp)) return;

• intel_pxp_tee_component_fini(pxp);
• destroy_vcs_context(pxp);
• }

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c new file mode 100644 index 000000000000..2f28f34c721d --- /dev/null +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright(c) 2020 Intel Corporation.
• */

+#include <linux/component.h> +#include "drm/i915_pxp_tee_interface.h" +#include "drm/i915_component.h" +#include "i915_drv.h" +#include "intel_pxp.h" +#include "intel_pxp_tee.h"

+static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev) +{

• return &kdev_to_i915(i915_kdev)->gt.pxp;

+}

+/**

• • i915_pxp_tee_component_bind - bind function to pass the function pointers to pxp_tee
• • @i915_kdev: pointer to i915 kernel device
• • @tee_kdev: pointer to tee kernel device
• • @data: pointer to pxp_tee_master containing the function pointers
• • This bind function is called during the system boot or resume from system sleep.
• • Return: return 0 if successful.
• */

+static int i915_pxp_tee_component_bind(struct device *i915_kdev,

• 		       struct device *tee_kdev, void *data)
  

+{

• struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
• pxp->pxp_component = data;
• pxp->pxp_component->tee_dev = tee_kdev;
• return 0;

+}

+static void i915_pxp_tee_component_unbind(struct device *i915_kdev,

• 			  struct device *tee_kdev, void *data)
  

+{

• struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
• pxp->pxp_component = NULL;

+}

+static const struct component_ops i915_pxp_tee_component_ops = {

• .bind = i915_pxp_tee_component_bind,
• .unbind = i915_pxp_tee_component_unbind,

+};

+int intel_pxp_tee_component_init(struct intel_pxp *pxp) +{

• int ret;
• struct intel_gt *gt = pxp_to_gt(pxp);
• struct drm_i915_private *i915 = gt->i915;
• ret = component_add_typed(i915->drm.dev, &i915_pxp_tee_component_ops,

• 		  I915_COMPONENT_PXP);
  

• if (ret < 0) {

• drm_err(&i915->drm, "Failed to add PXP component (%d)\n", ret);
  

• return ret;
  

• }
• pxp->pxp_component_added = true;
• return 0;

+}

+void intel_pxp_tee_component_fini(struct intel_pxp *pxp) +{

• struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
• if (fetch_and_zero(&pxp->pxp_component_added))

+Daniel.

If I'm understanding his locking guidelines well, I believe we should start avoiding even these functions.

If there's a risk of init and fini functions to conflict we should use regular mutexes.

In case they are called from obvious probe unprobe places than we could simply set to false and del the component. But if you have to add this here on this latest revision to fix a bug I'm assuming we will need a proper mutex.

There is no risk of conflict here, but since we don't abort driver init if pxp_init fails, we can end up calling pxp_fini on driver unload even if pxp_init was unsuccessful, so we need a variable to track the init state of the component. The bug I fixed was actually that reusing pxp->pxp_component for this tracking doesn't work, because the PCI subsystem unbinds the component on suspend, which clears pxp->pxp_component; the component itself is still there even if not currently bound and needs removal, hence the use of a different variable.

Daniele

...

• component_del(i915->drm.dev, &i915_pxp_tee_component_ops);
  

+} diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h new file mode 100644 index 000000000000..23d050a5d3e7 --- /dev/null +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: MIT */ +/*

• • Copyright(c) 2020, Intel Corporation. All rights reserved.
• */

+#ifndef __INTEL_PXP_TEE_H__ +#define __INTEL_PXP_TEE_H__

+#include "intel_pxp.h"

+int intel_pxp_tee_component_init(struct intel_pxp *pxp); +void intel_pxp_tee_component_fini(struct intel_pxp *pxp);

+#endif /* __INTEL_PXP_TEE_H__ */ diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h index bd12c520e60a..3a8e17e591bd 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h @@ -6,9 +6,15 @@ #ifndef __INTEL_PXP_TYPES_H__ #define __INTEL_PXP_TYPES_H__

+#include <linux/types.h>

• struct intel_context;

+struct i915_pxp_component;

struct intel_pxp {

• struct i915_pxp_component *pxp_component;
• bool pxp_component_added;
• struct intel_context *ce; };

-- 2.25.1