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

Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> · Tue, 31 Aug 2021 14:17:44 -0700

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

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@xxxxxxxxx>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> #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








